Skip to content
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

Speed up Time.zone.now #26359

Merged
merged 1 commit into from Oct 5, 2016
Merged

Speed up Time.zone.now #26359

merged 1 commit into from Oct 5, 2016

Conversation

@maclover7
Copy link
Member

@maclover7 maclover7 commented Sep 1, 2016

@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

Pretty... pretty... pretty good.

@KevinBongart
KevinBongart reviewed Sep 2, 2016
View changes
activesupport/lib/active_support/time_with_zone.rb Outdated
@@ -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)

This comment has been minimized.

@KevinBongart

KevinBongart Sep 2, 2016

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 comment has been minimized.

@natematykiewicz

natematykiewicz Sep 3, 2016
Contributor

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?

This comment has been minimized.

@maclover7

maclover7 Sep 3, 2016
Author Member

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

@maclover7 maclover7 force-pushed the maclover7:jm-speed-up-time branch Sep 3, 2016
@natematykiewicz
natematykiewicz reviewed Sep 6, 2016
View changes
activesupport/lib/active_support/time_with_zone.rb Outdated
@@ -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?

This comment has been minimized.

@natematykiewicz

natematykiewicz Sep 6, 2016
Contributor

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?

This comment has been minimized.

@KevinBongart

KevinBongart Sep 6, 2016

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

This comment has been minimized.

@natematykiewicz

natematykiewicz Sep 6, 2016
Contributor

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, 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 maclover7 force-pushed the maclover7:jm-speed-up-time branch to 353122c Oct 2, 2016
@maclover7
Copy link
Member Author

@maclover7 maclover7 commented Oct 2, 2016

@matthewd matthewd merged commit 0464b72 into rails:master Oct 5, 2016
2 checks passed
2 checks passed
codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maclover7 maclover7 deleted the maclover7:jm-speed-up-time branch Oct 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.