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

Fix TimeZone.p.getXxxTransition() worst-case perf #134

Merged
merged 1 commit into from
Feb 12, 2022

Conversation

justingrant
Copy link
Contributor

@justingrant justingrant commented Feb 8, 2022

When passed very far-past or very far-future dates,TimeZone.p.get(Next|Previous)Transition() currently takes minutes of 100% CPU to finish because those methods include a loop that makes expensive system calls for every 2 weeks until it finds a transition. For periods where there are no transitions, this loop will run millions of times which is time consuming.

Note that native implementations have access to the TZDB data directly so this is not an issue there. We could in theory bundle TZDB data (or simply bundle a shorter list of per-timezone data showing the earliest and oldest TZDB rules for each) but doing so would add a lot of complexity (and at least some bundle size) to this polyfill for minimal benefit, so solving the problem with optimization is, IMHO, preferable.

This PR improves worst-case perf ~1500x by:

  • Skipping looping for any dates before 1847 CE, which is the first entry in the TZDB.
  • Optimizing handling of dates 10 years or later than the current system time. Offset changes are only planned a few years in advance, so when provided a far-future date, we only need to check one year away to see if there's DST projected forward from the most recent rule in that TZ. If there's not, then when calling getNextTransition we know there's no transitions (and can return null) and when calling getPreviousTransition we can skip all the way back to current time + 10 years because we know that no transitions will be in the skipped period.

These optimizations reduce a worst-case loop of 273K years to 175 years, or about 1500x faster. That translates to a worst-case call time of ~80ms on my older MacBook Pro, which is still not great but is probably good enough for now given this is a corner case of an unusual API.

Fixes #60.

@justingrant justingrant changed the title Fix TimeZone.p.getXxxTransition() worst-case perf Fix TimeZone.p.getXxxTransition() worst-case perf Feb 8, 2022
@justingrant justingrant requested review from 12wrigja and ptomato and removed request for 12wrigja February 8, 2022 22:42
@justingrant justingrant force-pushed the get-transition-perf branch 2 times, most recently from 7142425 to d328bd8 Compare February 9, 2022 01:42
@justingrant
Copy link
Contributor Author

Given that this is a perf PR, I figured it'd make sense to add actual perf tests for the changes. Actual worst-case call perf is about 80ms on my older MacBook Pro, so just to be safe in CI I bumped it up to 800ms which hopefully should be enough for slow CI systems.

If this kind of test is a bad idea, then I'm happy to revert.

Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems clever!

I'm personally not a fan of the "must take less than 800 ms" tests, but I'll defer to you and James on that. If I were setting it up, I'd probably want to have it run on PRs only, and compare the performance before and after the change.

I'd still love to see an approach based on tc39/proposal-temporal#1370 when the time is right!

Copy link
Contributor

@12wrigja 12wrigja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be interesting to be able to measure this in terms of the number of operations happening - I wonder if the JSBI static functions could be spied on and called through, and all the spies call counts summed up to give a useful measurement.

That being said, I have no qualms with checking in your current tests, and if they end up being flaky we can always disable them.

test/timezone.mjs Outdated Show resolved Hide resolved
test/timezone.mjs Outdated Show resolved Hide resolved
When passed vary far-past or far-future dates,
`TimeZone.p.get(Next|Previous)Transition()` would take minutes of 100%
CPU to finish. This commit improves worst-case perf more than 1000x by:
* Skipping looping for any dates before 1847 CE, which is the first
  entry in the TZDB.
* Optimizing handling of dates 10 years or later than the current system
  time. Offset changes are only planned a few years in advance, so when
  provided a far-future date, we only need to check one year away to see
  if there's DST projected forward from the most recent rule in that TZ.
  If there's not, then when calling `getPreviousTransition` we can skip
  all the way back to current time + 10 years because no transitions
  will be in the skipped period.

These optimizations reduce a worst-case loop of over 200K years to
under 200 years, for a more-than-1000X worst-case perf improvement.
@justingrant
Copy link
Contributor Author

It would be interesting to be able to measure this in terms of the number of operations happening - I wonder if the JSBI static functions could be spied on and called through, and all the spies call counts summed up to give a useful measurement.

Seems reasonable to investigate, but I won't have time to do it! 😄

That being said, I have no qualms with checking in your current tests, and if they end up being flaky we can always disable them.

Sounds good. After fixing the typos you found then I'll merge this PR, and we can always remove the tests if they're problematic later.

@justingrant justingrant merged commit e70d632 into js-temporal:main Feb 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TimeZone GetNextTransition / GetPreviousTransition for far-future and far-past dates
3 participants