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

Duration normalization, part 2 of 3 #2727

Merged
merged 5 commits into from
Jan 31, 2024
Merged

Duration normalization, part 2 of 3 #2727

merged 5 commits into from
Jan 31, 2024

Conversation

ptomato
Copy link
Collaborator

@ptomato ptomato commented Nov 16, 2023

(Note: stacked on top of #2722, will rebase as appropriate)

This includes everything from #2612 except the last commit, which I'm still checking for a potential bug. These are the changes that place the limits on each component of a Temporal.Duration, and implement the internal calculations in a "normalized form" - 32-bit years, months, weeks, and time units expressed as one <96-bit integer.

Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Attention: 30 lines in your changes are missing coverage. Please review.

Comparison is base (35732cb) 96.73% compared to head (66a1b45) 96.58%.
Report is 1 commits behind head on main.

❗ Current head 66a1b45 differs from pull request most recent head 7ee77af. Consider uploading reports for the commit 7ee77af to get more accurate results

Files Patch % Lines
polyfill/lib/timeduration.mjs 93.24% 10 Missing ⚠️
polyfill/lib/duration.mjs 89.41% 9 Missing ⚠️
polyfill/lib/ecmascript.mjs 97.96% 4 Missing and 5 partials ⚠️
polyfill/lib/math.mjs 96.36% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2727      +/-   ##
==========================================
- Coverage   96.73%   96.58%   -0.16%     
==========================================
  Files          21       23       +2     
  Lines       12444    12226     -218     
  Branches     2255     2280      +25     
==========================================
- Hits        12038    11808     -230     
- Misses        349      359      +10     
- Partials       57       59       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ptomato ptomato mentioned this pull request Nov 16, 2023
91 tasks
polyfill/lib/ecmascript.mjs Outdated Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
polyfill/lib/ecmascript.mjs Show resolved Hide resolved
polyfill/lib/ecmascript.mjs Outdated Show resolved Hide resolved
@ptomato ptomato force-pushed the duration-normalize-part-2 branch 4 times, most recently from b9f2c58 to 0e2f662 Compare November 29, 2023 00:26
spec/zoneddatetime.html Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
spec/zoneddatetime.html Outdated Show resolved Hide resolved
spec/zoneddatetime.html Outdated Show resolved Hide resolved
spec/plaindatetime.html Outdated Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
spec/duration.html Outdated Show resolved Hide resolved
@ptomato
Copy link
Collaborator Author

ptomato commented Jan 16, 2024

Thanks for the comments, @anba. I've addressed some of them and pushed up an interim version of the PR. I'll continue addressing the remaining ones in the next few days.

In order to avoid unbounded integer arithmetic, we place an upper bound on
the total of the time portion of a duration (days through nanoseconds).

For the purpose of determining the limits, days are always 24 hours, even
if a calendar day may be a different number of hours relative to a
particular ZonedDateTime.

It's now no longer possible to make Duration.prototype.total() come up
with an infinite result; removing the loops in UnbalanceDurationRelative
made it so that the duration's calendar units must be able to be added to
a relativeTo date without overflowing, and this change makes it so that
the duration's time units are too small to overflow to infinity either.
This introduces Normalized Time Duration Records, which we use to
encapsulate 96-bit integer operations on duration times. (In the reference
polyfill, the TimeDuration class fulfills the same purpose.) These
operations are specified naively in the mathematical value domain, but can
be changed in a later editorial commit to correspond to how
implementations would write 64+32 bit operations, if we so desire. (The
results must be exactly the same, so that can be decided later, outside of
a TC39 plenary.)

This commit also replaces TotalDurationNanoseconds with
NormalizeTimeDuration, and NanosecondsToDays with
NormalizedTimeDurationToDays. Several operations are changed to return a
Normalized Duration Record, which is a Normalized Time Duration record
combined with a Date Duration Record.

Having already limited time units of durations in the previous commit,
this does not affect any results, nor any existing tests in test262. But I
can't prove conclusively that there isn't some edge case somewhere that
makes this change observable.

(also obsoletes several pre-existing editorial mistakes)
Closes: #2536
Closes: #2638
Closes: #2616
Carefully crafted custom time zones can cause NormalizedTimeDurationToDays
to calculate a day length that is arbitrarily long. In order for
implementations not to have to use a normalized time duration or bigint to
represent the day length in nanoseconds, limit it to less than 2⁵³ ns.
This way, it can be stored in a 64-bit float.

This allows time zones to specify day lengths of up to ~104.25 real
24-hour days, which should be more than enough.

This also ensures that the number of days returned from
NormalizedTimeDurationToDays is within the limit of 2⁵³/86400, which
allows implementations to use 64-bit integer addition in RoundDuration.
In order to prevent having to use bigint arithmetic, limit years, months,
and weeks to 32 bits each in durations.

There are more changes to the reference code than to the spec in this
commit because the upper limit now allows us to rewrite the reference
code's RoundDuration algorithm in a way that's more similar to how it was
already specified in the spec text.
@ptomato
Copy link
Collaborator Author

ptomato commented Jan 31, 2024

I believe all comments have been addressed and tests are complete now (tc39/test262#3961). (They are still showing up as failing, because the CI runs them against the pinned commit of test262, but they pass against that branch.)

The change has had scrutiny from several people (either as part of #2612 or on this PR thread) and is just missing a review from someone with merge privileges.

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.

4 participants