From fa9f1538f034667ea5d55838bd46844642b54274 Mon Sep 17 00:00:00 2001
From: Ryan De Villa <ryand@nulogy.com>
Date: Mon, 18 Jul 2016 13:55:15 -0400
Subject: Fix performance regression in `TimeWithZone#to_time`
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

A performance regression was introduced by commit b79adc4323ff289aed3f5787fdfbb9542aa4f89f
from Rails 4.0.0.beta1, in which `TimeWithZone#to_time` no longer returns a
cached instance attribute but instead coerces the value to `Time`. This
coerced value is not cached, and recomputation degrades the performance
of comparisons between TimeWithZone objects.

See https://github.com/rails/rails/commit/b79adc4323ff289aed3f5787fdfbb9542aa4f89f#diff-3497a506c921a3a3e40fd517e92e4fe3R322
for the change in question.

The following benchmark, which reverts the change linked above, demonstrates
the performance regression:

    require 'active_support/time'
    require 'benchmark/ips'

    utc = Time.utc(2000, 1, 1, 0)
    time_zone = ActiveSupport::TimeZone['Eastern Time (US & Canada)']

    twz = ActiveSupport::TimeWithZone.new(utc, time_zone)
    twz2 = ActiveSupport::TimeWithZone.new(Time.utc(1999, 12, 31, 23, 59, 59), ActiveSupport::TimeZone['UTC'])

    patchedTimeWithZone = Class.new(ActiveSupport::TimeWithZone) do
      def to_time
        utc
      end
    end

    patched_twz = patchedTimeWithZone.new(utc, time_zone)
    patched_twz2 = patchedTimeWithZone.new(Time.utc(1999, 12, 31, 23, 59, 59), ActiveSupport::TimeZone['UTC'])

    Benchmark.ips do |x|
      x.report("comparison out of the box") { twz <=> twz2 }
      x.report("comparison reverting to_time") { patched_twz <=> patched_twz2 }
      x.compare!
    end

The results, when run in rails-dev-box, are as follows:

    Warming up --------------------------------------
    comparison out of the box
                            24.765k i/100ms
    comparison reverting to_time
                            57.237k i/100ms
    Calculating -------------------------------------
    comparison out of the box
                            517.245k (± 4.7%) i/s -      2.600M in   5.038700s
    comparison reverting to_time
                              2.624M (± 5.0%) i/s -     13.050M in   4.985808s

    Comparison:
    comparison reverting to_time:  2624266.1 i/s
    comparison out of the box:   517244.6 i/s - 5.07x slower

The change made to run the benchmark, however, is not possible, as it would
undo the intent to standardize the return value of `to_time` to `Time` in
the system timezone.

Our proposed solution is to restore the caching behaviour of `to_time`
as it existed prior to the change linked above.

Benchmark of our solution:

    require 'active_support/time'
    require 'benchmark/ips'

    patchedTimeWithZone = Class.new(ActiveSupport::TimeWithZone) do
      def to_time
        @to_time ||= super
      end
    end

    utc = Time.utc(2000, 1, 1, 0)
    time_zone = ActiveSupport::TimeZone['Eastern Time (US & Canada)']

    twz = ActiveSupport::TimeWithZone.new(utc, time_zone)
    twz2 = ActiveSupport::TimeWithZone.new(Time.utc(1999, 12, 31, 23, 59, 59), ActiveSupport::TimeZone['UTC'])

    patched_twz = patchedTimeWithZone.new(utc, time_zone)
    patched_twz2 = patchedTimeWithZone.new(Time.utc(1999, 12, 31, 23, 59, 59), ActiveSupport::TimeZone['UTC'])

    Benchmark.ips do |x|
      x.report("TimeWithZone comparison - existing implementation") { twz <=> twz2 }
      x.report("TimeWithZone comparison - caching implementation") { patched_twz <=> patched_twz2 }
      x.compare!
    end

Results in rails-dev-box:

    Warming up --------------------------------------
    TimeWithZone comparison - existing implementation
                            26.629k i/100ms
    TimeWithZone comparison - caching implementation
                            59.144k i/100ms
    Calculating -------------------------------------
    TimeWithZone comparison - existing implementation
                            489.757k (± 4.2%) i/s -      2.450M in   5.011639s
    TimeWithZone comparison - caching implementation
                              2.802M (± 5.3%) i/s -     13.958M in   4.996116s

    Comparison:
    TimeWithZone comparison - caching implementation:  2801519.1 i/s
    TimeWithZone comparison - existing implementation:   489756.7 i/s - 5.72x slower
---
 activesupport/lib/active_support/time_with_zone.rb | 6 ++++++
 1 file changed, 6 insertions(+)

(limited to 'activesupport/lib/active_support/time_with_zone.rb')

diff --git a/activesupport/lib/active_support/time_with_zone.rb b/activesupport/lib/active_support/time_with_zone.rb
index 9d1147620c..f91225289d 100644
--- a/activesupport/lib/active_support/time_with_zone.rb
+++ b/activesupport/lib/active_support/time_with_zone.rb
@@ -84,6 +84,12 @@ module ActiveSupport
     end
     alias_method :getlocal, :localtime
 
+    def to_time_with_caching
+      @to_time ||= to_time_without_caching
+    end
+    alias_method :to_time_without_caching, :to_time
+    alias_method :to_time, :to_time_with_caching
+
     # Returns true if the current time is within Daylight Savings Time for the
     # specified time zone.
     #
-- 
cgit v1.2.3