Reduce memory by not duplicating an argument #404
Conversation
I spotted this in a memory_profiler report for some of my app's specs.
timecop was showing up in the list of gems with the most allocated memory.
This dup appears unnecessary. The code I can see isn't making
modifications to the object, and I would expect `at` to return an new
object anyways.
memory_profiler before:
allocated memory by location
-----------------------------------
...
49_836_480 /Users/josh.nichols/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/timecop-0.9.6/lib/timecop/time_stack_item.rb:59
...
allocated objects by location
-----------------------------------
...
934_434 /Users/josh.nichols/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/timecop-0.9.6/lib/timecop/time_stack_item.rb:59
...
memory_profiler after:
allocated memory by location
-----------------------------------
...
26_997_696 /Users/josh.nichols/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/timecop-0.9.6/lib/timecop/time_extensions.rb:79
...
allocated objects by location
-----------------------------------
662_346 /Users/josh.nichols/.rbenv/versions/3.2.2/lib/ruby/gems/3.2.0/gems/timecop-0.9.6/lib/timecop/time_stack_item.rb:59
This decreased memory allocated in my tests by 45%, and objects
allocated by 29%.
| @@ -56,7 +56,7 @@ def scaling_factor | |||
|
|
|||
| def time(time_klass = Time) #:nodoc: | |||
| if @time.respond_to?(:in_time_zone) | |||
| time = time_klass.at(@time.dup.localtime) | |||
|
@technicalpickles Thanks for the thoughtful PR. I've reviewed it and will merge it now. It's interesting, I was curious why we had the dup in the first place and I traced it back to this commit 27dc330 And that commit is from this PR #107 It looks like that dup is no longer necessary since we don't call the UTC method anymore... So since this commit 18474ab the dup isn't giving us anything. 👏 |
|
@joshuacronemeyer thanks! I started looking at the git history, but didn't get back far enough where it was first used (rather than just moving it around) Hm, I'm looking more closely at The test added in #107 is still around, and passed though, so it must have had different behavior previously. |
|
Hmm. That's interesting. Yea hopefully we're not asking for trouble here, but I also am looking to that test to tell me that we're in the clear on this. One interesting thing is that it has to be a difference in the way we call it, not the behavior of ruby because our build runs a bunch of really old rubies. |
|
@joshuacronemeyer Hi, this fix changes irb(main):001:0> t1 = Time.now.utc
=> 2023-08-14 01:47:16.918427 UTC
irb(main):002:0> t2 = Time.now.utc
=> 2023-08-14 01:47:21.596669 UTC
irb(main):003:0> t1.localtime
=> 2023-08-14 10:47:16.918427 +0900
irb(main):004:0> t2.dup.localtime
=> 2023-08-14 10:47:21.596669 +0900
irb(main):005:0> t1
=> 2023-08-14 10:47:16.918427 +0900
irb(main):006:0> t2
=> 2023-08-14 01:47:21.596669 UTC |
|
@joshuacronemeyer sounds like reverting is called for. I can follow up with another attempt with better tests. |
|
@nagachika @citystar @technicalpickles i released a new version that reverts this code. Please let me know if the issue is resolved. @citystar I'd really appreciate it if you could provide enough of the code from your failing test for us to reproduce this issue. We have a test for this scenario but apparently it does not work 😿 |
|
@citystar oops. Sorry i see your earlier comment. That is what we need. Thanks again. |
|
@joshuacronemeyer Thank you for the quick fix. The new version works perfectly at our side. |
I spotted this in a memory_profiler report for some of my app's specs. timecop was showing up in the list of gems with the most allocated memory.
This dup appears unnecessary. The code I can see isn't making modifications to the object, and I would expect
atto return an new object anyways.memory_profiler before:
memory_profiler after:
This decreased memory allocated in my tests by 45%, and objects allocated by 29%.