New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix performance regression in `TimeWithZone#to_time` #25880

Merged
merged 2 commits into from Aug 30, 2016

Conversation

Projects
None yet
7 participants
@ryandv
Contributor

ryandv commented Jul 18, 2016

A performance regression was introduced by commit b79adc4
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 b79adc4#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
@clemensp

This comment has been minimized.

clemensp commented Jul 18, 2016

We would like to have this back-ported to Rails 4.2 as well.

Ideally, it would be back-ported to Rails 4.1, where we are currently experiencing a big performance degradation, but I assume that it is not an option anymore.

@arturopie

This comment has been minimized.

Contributor

arturopie commented Jul 18, 2016

👍

@guilleiguaran

This comment has been minimized.

Member

guilleiguaran commented Jul 19, 2016

Sorry, this can't be backported to Rails 4.1 but we will consider to do it to 4.2

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jul 20, 2016

@clemensp

This comment has been minimized.

clemensp commented Aug 4, 2016

@rafaelfranca What would you say is an acceptable waiting period before asking for updates? 😃
I'm not sure what the standard wait time for Rails PRs is.

In any case, I'd like to see if the fix looks reasonable, so that I can replace some workarounds.

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Aug 5, 2016

@clemensp sorry, day job got in the way

@ryandv why the separate method and aliasing? Why not caching in the to_time method directly? e.g:

def to_time
  @to_time ||= utc.to_time
end

Also you may want to double check there's no way of mutating an AS::TWZ instance - I don't think so but best to be sure.

@ryandv

This comment has been minimized.

Contributor

ryandv commented Aug 5, 2016

@pixeltrix We were following existing conventions within ActiveSupport for overriding methods, as exemplified at https://github.com/rails/rails/blob/5-0-stable/activesupport/lib/active_support/core_ext/date/calculations.rb#L141

However, we are not attached to either approach and will happily follow your suggestion.

We did identify a mutable configuration option to_time_preserves_timezone, which could cause the utc_offset used to localize the value of to_time to fall out of sync with the memoized attribute. However, the intended usage of this option appears to be as an initialization configuration.

See c9c5788 for relevant documentation.

@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Aug 5, 2016

@ryandv yes, I think to_time_preserves_timezone shouldn't be a problem. Thinking about it the caching probably needs to implemented in the localtime method since that's what to_time eventually calls, e.g:

def localtime(utc_offset = nil)
  @localtime ||= utc.getlocal(utc_offset)
end
alias_method :getlocal, :localtime

A quick check here shows all the Active Support tests pass

ryandv added some commits Jul 18, 2016

Fix performance regression in `TimeWithZone#to_time`
A performance regression was introduced by commit b79adc4
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 b79adc4#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
@ryandv

This comment has been minimized.

Contributor

ryandv commented Aug 23, 2016

As per your suggestions, we've removed method aliasing and moved the memoization
to TimeWithZone#localtime. Both our original changes and your suggested
revisions address the performance regression. However, our original
fix is directly backportable to 4-2-stable. This is because localtime
is not along the codepath used by Time#compare_with_coercion in 4-2-stable.

Compare the following:

The following benchmark validates that the revised changeset still performs
as expected:

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

patchedTimeWithZone = Class.new(ActiveSupport::TimeWithZone) do
  def getlocal
    @getlocal ||= 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

The results, in rails-dev-box, compared against 58e0b1d:

vagrant@rails-dev-box:/vagrant/rails$ bundle exec ruby cached_localtime_performance.rb
Warming up --------------------------------------
TimeWithZone comparison - existing implementation
                        25.925k i/100ms
TimeWithZone comparison - caching implementation
                        53.263k i/100ms
Calculating -------------------------------------
TimeWithZone comparison - existing implementation
                        446.104k (±12.5%) i/s -      2.204M in   5.043571s
TimeWithZone comparison - caching implementation
                          2.128M (± 5.7%) i/s -     10.599M in   4.998911s

Comparison:
TimeWithZone comparison - caching implementation:  2127709.3 i/s
TimeWithZone comparison - existing implementation:   446103.9 i/s - 4.77x slower

@ryandv ryandv force-pushed the ryandv:fix_performance_regression_in_timewithzone_to_time branch to af054a5 Aug 23, 2016

@pixeltrix pixeltrix merged commit 3132fa6 into rails:master Aug 30, 2016

2 checks passed

codeclimate Code Climate has skipped analysis of this commit.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pixeltrix

This comment has been minimized.

Member

pixeltrix commented Aug 30, 2016

@ryandv thanks!

pixeltrix added a commit that referenced this pull request Aug 30, 2016

Merge pull request #25880 from ryandv/fix_performance_regression_in_t…
…imewithzone_to_time

Fix performance regression in `TimeWithZone#to_time`
(cherry picked from commit 3132fa6)

pixeltrix added a commit that referenced this pull request Aug 31, 2016

Cache to_time and to_datetime for performance
As demonstrated in #25880 the to_time method converts the utc time
instance to a local time instance which is an expensive operation
so since ActiveSupport::TimeWithZone is a value object and shouldn't
be mutated we can safely cache it without worrying about invalidation.

pixeltrix added a commit that referenced this pull request Sep 1, 2016

Cache to_datetime for performance
As demonstrated in #25880 the to_time method converts the utc time
instance to a local time instance which is an expensive operation.
Since to_datetime involves similar expensive operations we should
also cache it to speed up comparison with lots of values.

pixeltrix added a commit that referenced this pull request Oct 2, 2016

Revert "Merge pull request #25880 from ryandv/fix_performance_regress…
…ion_in_timewithzone_to_time"

Turns out trying to cache on localtime with arguments is too hard
so we'll do it on DateAndTime::Compatibility#to_time instead.

This reverts commit 3132fa6, reversing
changes made to 6949f8e.

pixeltrix added a commit that referenced this pull request Oct 2, 2016

Cache to_time to improve performance when comparing
In #25880 we tried to cache localtime to fix the performance
regression but that proved to be difficult due to the fact that
localtime/getlocal can take a utc_offset argument. We tried
caching based on the argument but since the argument can be nil
sometimes that meant that if the TZ environment variable changed
then the cached value for nil became invalid. By moving the
caching to DateAndTime#compatibility we don't have to worry about
arguments since it doesn't take any.

There is a possible edge condition where preserve_timezone is set
to false and the system timezone changes then it could result in
a cached value being incorrect but the only way to fix this would
be to remove all caching and live with the performance issue.

pixeltrix added a commit that referenced this pull request Oct 2, 2016

Revert "Merge pull request #25880 from ryandv/fix_performance_regress…
…ion_in_timewithzone_to_time"

Turns out trying to cache on localtime with arguments is too hard
so we'll do it on DateAndTime::Compatibility#to_time instead.

This reverts commit 0054e02.

pixeltrix added a commit that referenced this pull request Oct 2, 2016

Cache to_time to improve performance when comparing
In #25880 we tried to cache localtime to fix the performance
regression but that proved to be difficult due to the fact that
localtime/getlocal can take a utc_offset argument. We tried
caching based on the argument but since the argument can be nil
sometimes that meant that if the TZ environment variable changed
then the cached value for nil became invalid. By moving the
caching to DateAndTime#compatibility we don't have to worry about
arguments since it doesn't take any.

There is a possible edge condition where preserve_timezone is set
to false and the system timezone changes then it could result in
a cached value being incorrect but the only way to fix this would
be to remove all caching and live with the performance issue.

(cherry picked from commit 9128ba5)

MichaelSp added a commit to MichaelSp/rails that referenced this pull request Nov 2, 2016

Cache to_time and to_datetime for performance
As demonstrated in rails#25880 the to_time method converts the utc time
instance to a local time instance which is an expensive operation
so since ActiveSupport::TimeWithZone is a value object and shouldn't
be mutated we can safely cache it without worrying about invalidation.

pixeltrix added a commit that referenced this pull request Jan 3, 2017

Cache to_time to improve performance when comparing
In #25880 we tried to cache localtime to fix the performance
regression but that proved to be difficult due to the fact that
localtime/getlocal can take a utc_offset argument. We tried
caching based on the argument but since the argument can be nil
sometimes that meant that if the TZ environment variable changed
then the cached value for nil became invalid. By moving the
caching to DateAndTime#compatibility we don't have to worry about
arguments since it doesn't take any.

There is a possible edge condition where preserve_timezone is set
to false and the system timezone changes then it could result in
a cached value being incorrect but the only way to fix this would
be to remove all caching and live with the performance issue.

(cherry picked from commit 9128ba5)

pixeltrix added a commit that referenced this pull request Jan 3, 2017

Cache to_time to improve performance when comparing
In #25880 we tried to cache localtime to fix the performance
regression but that proved to be difficult due to the fact that
localtime/getlocal can take a utc_offset argument. We tried
caching based on the argument but since the argument can be nil
sometimes that meant that if the TZ environment variable changed
then the cached value for nil became invalid. By moving the
caching to DateAndTime#compatibility we don't have to worry about
arguments since it doesn't take any.

There is a possible edge condition where preserve_timezone is set
to false and the system timezone changes then it could result in
a cached value being incorrect but the only way to fix this would
be to remove all caching and live with the performance issue.

(cherry picked from commit 9128ba5)

pixeltrix added a commit that referenced this pull request Jan 4, 2017

Cache to_time to improve performance when comparing
In #25880 we tried to cache localtime to fix the performance
regression but that proved to be difficult due to the fact that
localtime/getlocal can take a utc_offset argument. We tried
caching based on the argument but since the argument can be nil
sometimes that meant that if the TZ environment variable changed
then the cached value for nil became invalid. By moving the
caching to DateAndTime#compatibility we don't have to worry about
arguments since it doesn't take any.

There is a possible edge condition where preserve_timezone is set
to false and the system timezone changes then it could result in
a cached value being incorrect but the only way to fix this would
be to remove all caching and live with the performance issue.

(cherry picked from commit 9128ba5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment