Speed up Time.zone.now #26359

maclover7 · 2016-09-01T22:18:23Z

@amatsuda, during his RailsConf talk this past year, presented a
benchmark that showed Time.zone.now (an Active Support joint)
performing 24.97x slower than Ruby's Time.now. Rails master appears to
be a bit faster than that, currently clocking in at 18.25x slower than
Time.now. Here's the exact benchmark data for that:

Warming up --------------------------------------
            Time.now   127.923k i/100ms
       Time.zone.now    10.275k i/100ms
Calculating -------------------------------------
            Time.now      1.946M (± 5.9%) i/s -      9.722M in   5.010236s
       Time.zone.now    106.625k (± 4.3%) i/s -    534.300k in   5.020343s

Comparison:
            Time.now:  1946220.1 i/s
       Time.zone.now:   106625.5 i/s - 18.25x slower

What if I told you we could make Time.zone.now even faster? Well,
that's exactly what this patch accomplishes. When creating ActiveSupport::TimeWithZone
objects, we try to convert the provided time to be in a UTC format. All
this patch does is, in the method where we convert a provided time to
UTC, check if the provided time is already UTC, and is a Time object
and then return early if that is the case, This sidesteps having to continue on,
and create a new Time object from scratch. Here's the exact benchmark
data for my patch:

Warming up --------------------------------------
            Time.now   124.870k i/100ms
       Time.zone.now    27.344k i/100ms
Calculating -------------------------------------
            Time.now      1.924M (± 5.6%) i/s -      9.615M in   5.011704s
       Time.zone.now    310.385k (± 4.3%) i/s -      1.559M in   5.030432s

Comparison:
            Time.now:  1923815.5 i/s
       Time.zone.now:   310384.7 i/s - 6.20x slower

With this patch, we go from Time.zone.now being 18.25x slower than
Time.now to only being 6.20x slower than Time.now. I'd obviously love some verification on this patch, since these numbers sound pretty interesting... :)

This is the benchmark-ips report I have been using while working on this:

require 'benchmark/ips'

Time.zone = 'Eastern Time (US & Canada)'

Benchmark.ips do |x|
  x.report('Time.now') {
    Time.now
  }

  x.report('Time.zone.now') {
    Time.zone.now
  }

  x.compare!
end

cc @amatsuda
cc performance folks @tenderlove and @schneems

KevinBongart · 2016-09-02T12:00:02Z

activesupport/lib/active_support/time_with_zone.rb

@@ -476,6 +476,8 @@ def get_period_and_ensure_valid_local_time(period)
      end

      def transfer_time_values_to_utc_constructor(time)
+        # avoid creating another Time object if possible
+        return time if time.utc? && time.is_a?(Time)


Shouldn't these two conditions be swapped? I'm assuming we wouldn't want to call utc? on an object that isn't a Time instance. Or am I misunderstanding the context? (very likely)

This method is currently aways returning a standard Ruby Time object, regardless of what type of duck-typed time class you pass into it. This PR would change that because is_a? includes inheritance.

transfer_time_values_to_utc_constructor(Time.zone.now) would now return an instance of ActiveSupport::TimeWithZone instead of Time like it currently does.

irb(main):011:0> Time.zone.now.class => ActiveSupport::TimeWithZone irb(main):012:0> Time.zone.now.is_a?(Time) => true irb(main):013:0> Time.zone.now.instance_of?(Time) => false

I think what you really want is the following:

return time if time.instance_of?(Time) && time.utc?

👍 👍 Thank you both for your comments -- going to update my commit!

natematykiewicz · 2016-09-06T04:27:59Z

activesupport/lib/active_support/time_with_zone.rb

@@ -476,6 +476,8 @@ def get_period_and_ensure_valid_local_time(period)
      end

      def transfer_time_values_to_utc_constructor(time)
+        # avoid creating another Time object if possible
+        return time if time.instance_of?(Time) && time.utc?


You're welcome! I realized one last thing just now.

Theinstance_of? call needs to have the Time class rooted by adding :: in front of the class name (just like they did a few lines down).

Since this method is within ActiveSupport::TimeWithZone, time.instance_of?(Time) would return true if time were a ActiveSupport::Time, ActiveSupport::TimeWithZone::Time, or a plain old Time. To prevent against this and ensure that only Time objects return true, the line should actually be:

return time if time.instance_of?(::Time) && time.utc?

@natematykiewicz Out of curiosity, why wouldn't we want to return true for these subclasses? Sounds convenient to me

To be honest, I have no idea how this code gets used. All I know is, if the goal is to make it do the same thing it currently does, but only create new instances if necessary, then you'd want to root the Time.

This method previously (before this PR) always returned a utc ::Time object. So it seems like you'd only want to short-circuit if you already have a utc ::Time object (and not a subclass or something that acts like a Time object).

Maybe someone with more experience with ActiveSupport::TimeWithZone could answer that question?

If you want it to work with subclasses and be more loose, then you could go back to what he originally had:

return time if time.is_a?(Time) && time.utc?

but, otherwise you'd want:

return time if time.instance_of?(::Time) && time.utc?

One thing worth noting is that line 468 already checks some of this:
@time = transfer_time_values_to_utc_constructor(@time) unless @time.utc?

I think the initialize method is the bigger concern in his performance considerations.

@amatsuda

@amatsuda, during his RailsConf talk this past year, presented a benchmark that showed `Time.zone.now` (an Active Support joint) performing 24.97x slower than Ruby's `Time.now`. Rails master appears to be a _bit_ faster than that, currently clocking in at 18.25x slower than `Time.now`. Here's the exact benchmark data for that: ``` Warming up -------------------------------------- Time.now 127.923k i/100ms Time.zone.now 10.275k i/100ms Calculating ------------------------------------- Time.now 1.946M (± 5.9%) i/s - 9.722M in 5.010236s Time.zone.now 106.625k (± 4.3%) i/s - 534.300k in 5.020343s Comparison: Time.now: 1946220.1 i/s Time.zone.now: 106625.5 i/s - 18.25x slower ``` What if I told you we could make `Time.zone.now` _even_ faster? Well, that's exactly what this patch accomplishes. When creating `ActiveSupport::TimeWithZone` objects, we try to convert the provided time to be in a UTC format. All this patch does is, in the method where we convert a provided time to UTC, check if the provided time is already UTC, and is a `Time` object and then return early if that is the case, This sidesteps having to continue on, and create a new `Time` object from scratch. Here's the exact benchmark data for my patch: ``` Warming up -------------------------------------- Time.now 124.136k i/100ms Time.zone.now 26.260k i/100ms Calculating ------------------------------------- Time.now 1.894M (± 6.4%) i/s - 9.434M in 5.000153s Time.zone.now 301.654k (± 4.3%) i/s - 1.523M in 5.058328s Comparison: Time.now: 1893958.0 i/s Time.zone.now: 301653.7 i/s - 6.28x slower ``` With this patch, we go from `Time.zone.now` being 18.25x slower than `Time.now` to only being 6.28x slower than `Time.now`. I'd obviously love some verification on this patch, since these numbers sound pretty interesting... :) This is the benchmark-ips report I have been using while working on this: ```ruby require 'benchmark/ips' Time.zone = 'Eastern Time (US & Canada)' Benchmark.ips do |x| x.report('Time.now') { Time.now } x.report('Time.zone.now') { Time.zone.now } x.compare! end ``` cc @amatsuda cc performance folks @tenderlove and @schneems ![Pretty... pretty... pretty good.](https://media.giphy.com/media/bWeR8tA1QV4cM/giphy.gif)

maclover7 · 2016-10-02T19:13:10Z

cc @pixeltrix

maclover7 added activesupport needs feedback labels Sep 1, 2016

KevinBongart reviewed Sep 2, 2016
View reviewed changes

maclover7 force-pushed the jm-speed-up-time branch from 65aa2a0 to bfb20e8 Compare September 3, 2016 13:46

natematykiewicz reviewed Sep 6, 2016
View reviewed changes

maclover7 force-pushed the jm-speed-up-time branch from bfb20e8 to 353122c Compare October 2, 2016 19:12

matthewd merged commit 0464b72 into rails:master Oct 5, 2016

maclover7 deleted the jm-speed-up-time branch October 5, 2016 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up Time.zone.now #26359

Speed up Time.zone.now #26359

maclover7 commented Sep 1, 2016 •

edited

KevinBongart Sep 2, 2016

natematykiewicz Sep 3, 2016

maclover7 Sep 3, 2016

natematykiewicz Sep 6, 2016 •

edited

KevinBongart Sep 6, 2016

natematykiewicz Sep 6, 2016

maclover7 commented Oct 2, 2016

Speed up Time.zone.now #26359

Speed up Time.zone.now #26359

Conversation

maclover7 commented Sep 1, 2016 • edited

KevinBongart Sep 2, 2016

Choose a reason for hiding this comment

natematykiewicz Sep 3, 2016

Choose a reason for hiding this comment

maclover7 Sep 3, 2016

Choose a reason for hiding this comment

natematykiewicz Sep 6, 2016 • edited

Choose a reason for hiding this comment

KevinBongart Sep 6, 2016

Choose a reason for hiding this comment

natematykiewicz Sep 6, 2016

Choose a reason for hiding this comment

maclover7 commented Oct 2, 2016

maclover7 commented Sep 1, 2016 •

edited

natematykiewicz Sep 6, 2016 •

edited