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

Polyfill: Looking one year ahead isn't long enough to assume no transitions after that #2564

Closed
justingrant opened this issue Apr 23, 2023 · 6 comments · Fixed by #2660
Closed
Assignees
Labels
no-spec-text PR can be ignored by implementors non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE!

Comments

@justingrant
Copy link
Collaborator

The optimization in the code below (from 20ef40b) probably isn't correct, because when a well-run country makes a planned time zone change (e.g. stopping or changing use of DST), those changes should be announced more than a year in advance to give time for software to be updated before the change happens.

Sadly, not all countries are well-run in that way, but I think we should be more conservative to avoid breaking Test262 tests if a well-run country decides to do this.

So I'd recommend changing it from one year to a few years, maybe 4-5 years? I strongly doubt that any country announces changes more than 5 years in advance, because politicians like to claim credit sooner! Honestly 2-3 years is probably enough, but the perf cost of a few years is small, so might as well go for 5 years.

export function GetNamedTimeZonePreviousTransition(id, epochNanoseconds) {
// Optimization: if the instant is more than a year in the future and there
// are no transitions between the present day and a year from now, assume
// there are none after
const now = SystemUTCEpochNanoSeconds();
const yearLater = now.plus(DAY_NANOS.multiply(366));
if (epochNanoseconds.gt(yearLater)) {
const prevBeforeNextYear = GetNamedTimeZonePreviousTransition(id, yearLater);
if (prevBeforeNextYear === null || prevBeforeNextYear.lt(now)) {
return prevBeforeNextYear;
}
}

@justingrant justingrant added non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE! no-spec-text PR can be ignored by implementors labels Apr 23, 2023
@gilmoreorless
Copy link
Contributor

gilmoreorless commented Apr 23, 2023

The longest lead time I've experienced for a DST change was for the Sydney 2000 Olympics. Looking back through the tzdb notes, the adjustment to the DST start date in NSW for August 2000 was officially announced in May 1999. That's roughly 15 months.

Given that was a one-off adjustment for a fixed-date event that had been years in the planning, I doubt there are many lead-in times longer than that.

@justingrant
Copy link
Collaborator Author

Well, the US changed DST in August 2005 to take effect in March 2007. https://www.timetemperature.com/tzus/daylight_saving_time_extended.shtml

The world is much more dependent now vs. then on IoT devices that need to know the correct time and are really hard (or impossible) to update, so I suspect that subsequent DST changes (at least in the US) may have even longer lead times.

In any case there's no significant downside to being a bit more conservative here.

@ptomato
Copy link
Collaborator

ptomato commented Apr 26, 2023

The optimization is more narrow than that. If a country makes changes to their DST rules or stops using DST more than one year in advance, that is not affected.

The only thing affected is if a region that doesn't currently use DST starts using it, and the change makes it into the TZDB more than one year in advance. I based the assumption on:

  • most of the world doesn't need to use DST in the first place
  • in the regions where it might be useful, DST is becoming less popular due to increased computerization and there are movements to abolish it

I agree this shortcut wouldn't be appropriate for a production polyfill, because the spec simply says "the first time zone transition after epochNanoseconds" and so any optimizations must be 100% correct in order for an implementation to be compliant. Clearly there is a case where this optimization wouldn't be correct; I just think it's really unlikely.

However, we specifically use this polyfill to run the test262 tests, which are not testing for correctness of time zone transitions (that's up to the TZDB). The downside to being more conservative here is that it makes the test262 tests take an unreasonable time to complete. Anyway, a production polyfill probably ought to use TZDB rules directly instead of relying on repeated uses of DateTimeFormat, which would make getNextTransition significantly more efficient (see #1370).

@justingrant
Copy link
Collaborator Author

Meeting 2023-08-17: @justingrant will test to see if increasing the lookahead to 3 years has a noticeable impact on Test262 runtime, and if not, then we'll make this minor change.

@justingrant justingrant self-assigned this Aug 17, 2023
@justingrant
Copy link
Collaborator Author

I tested this. On my machine, extending the lookahead time from 1 year to 3 years adds about 2%-3% to the execution time of the transition-at-instant-boundaries.js test for getNextTransition, and adds about 3%-4% to the execution time of the corresponding test for getPreviousTransition. Actual elapsed time increases about 200ms for each test.

This seems small enough to be worth a PR, and we can decide there if 2%-4% is too much of a price to pay.

@justingrant
Copy link
Collaborator Author

justingrant commented Aug 19, 2023

Also, GetNamedTimeZoneNextTransition seems to have a bug where if you give it a date more than one year in the future, it never reports any transitions after that. This seems bad, and easy to hit even in the browser playground. I'lll fix that in the same PR so that any date will look fwd 3 years after the input date or the current date (whichever is later).

Temporal.TimeZone.from('America/New_York').getNextTransition(Temporal.Instant.from('2025-01-01T00:00Z'))
// => expected: some transition
// => actual: null

justingrant added a commit to justingrant/proposal-temporal that referenced this issue Aug 19, 2023
GetNamedTimeZonePreviousTransition currently has an optimization to
avoid fruitlessly searching for a transition for far-future dates. This
optimization ignores cases where a time zone's offset changes are
scheduled more than one year in advance.  This commit extends the
lookahead period to three years.

Similarly, GetNamedTimeZoneNextTransition has an optimization
where it assumes that no time zone has any transitions more than
one year in the future. This commit extends this lookahead to three
years later than either the current date or the input instant, whichever
is later.

When testing locally, these changes slow down these AOs by 2%-4%,
which IMO is an OK tradeoff for more accuracy.

Fixes tc39#2564
ptomato pushed a commit that referenced this issue Aug 21, 2023
GetNamedTimeZonePreviousTransition currently has an optimization to
avoid fruitlessly searching for a transition for far-future dates. This
optimization ignores cases where a time zone's offset changes are
scheduled more than one year in advance.  This commit extends the
lookahead period to three years.

Similarly, GetNamedTimeZoneNextTransition has an optimization
where it assumes that no time zone has any transitions more than
one year in the future. This commit extends this lookahead to three
years later than either the current date or the input instant, whichever
is later.

When testing locally, these changes slow down these AOs by 2%-4%,
which IMO is an OK tradeoff for more accuracy.

Fixes #2564
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-spec-text PR can be ignored by implementors non-prod-polyfill THIS POLYFILL IS NOT FOR PRODUCTION USE!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants