search

travisjeffery / timecop

A gem providing "time travel", "time freezing", and "time acceleration" capabilities, making it simple to test time-dependent code. It provides a unified method to mock Time.now, Date.today, and DateTime.now in a single call.
MIT License
3.36k stars 223 forks source link

Reduce memory by not duplicating an argument #404

Closed technicalpickles closed 1 year ago

technicalpickles commented 1 year ago

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%.

joshuacronemeyer commented 1 year ago

@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 https://github.com/travisjeffery/timecop/commit/27dc330fde5fa7407cb25a270edaa7926d53bb5f And that commit is from this PR https://github.com/travisjeffery/timecop/pull/107

It looks like that dup is no longer necessary since we don't call the UTC method anymore... So since this commit https://github.com/travisjeffery/timecop/commit/18474ab3b7508dc24bd0216c57b661191b82f9d7 the dup isn't giving us anything.

👏

technicalpickles commented 1 year ago

@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 localtime, it's calling utc.utc.getlocal(utc_offset), and utc does:

  @utc ||= incorporate_utc_offset(@time, -utc_offset)

The test added in #107 is still around, and passed though, so it must have had different behavior previously.

joshuacronemeyer commented 1 year ago

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.

citystar commented 1 year ago

@joshuacronemeyer Hi, this fix changes @time's timezone. Is it expected? Do we have an option to avoid it?

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
technicalpickles commented 1 year ago

@joshuacronemeyer sounds like reverting is called for. I can follow up with another attempt with better tests.

joshuacronemeyer commented 1 year ago

@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 😿

joshuacronemeyer commented 1 year ago

@citystar oops. Sorry i see your earlier comment. That is what we need. Thanks again.

citystar commented 1 year ago

@joshuacronemeyer Thank you for the quick fix. The new version works perfectly at our side.