Duration normalization, part 2 of 3 #2727
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Nov 16, 2023
Codecov ReportAttention:
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. |
91 tasks
ptomato
force-pushed
the
duration-normalize-part-2
branch
from
0620aed
to
8b6cbab
Compare
anba
reviewed
Nov 20, 2023
anba
reviewed
Nov 22, 2023
anba
reviewed
Nov 23, 2023
ptomato
force-pushed
the
duration-normalize-part-2
branch
4 times, most recently
from
b9f2c58
to
0e2f662
Compare
anba
reviewed
Dec 4, 2023
anba
reviewed
Dec 4, 2023
anba
reviewed
Dec 4, 2023
anba
reviewed
Dec 4, 2023
anba
reviewed
Dec 4, 2023
anba
reviewed
Dec 4, 2023
anba
reviewed
Dec 4, 2023
anba
reviewed
Dec 7, 2023
anba
reviewed
Dec 11, 2023
anba
reviewed
Dec 11, 2023
ptomato
force-pushed
the
duration-normalize-part-2
branch
from
0e2f662
to
08722d4
Compare
|
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. |
ptomato
force-pushed
the
duration-normalize-part-2
branch
2 times, most recently
from
be83c49
to
89235f9
Compare
ptomato
force-pushed
the
duration-normalize-part-2
branch
from
89235f9
to
854f5e1
Compare
ptomato
force-pushed
the
duration-normalize-part-2
branch
from
854f5e1
to
21653be
Compare
Ms2ger
force-pushed
the
duration-normalize-part-2
branch
2 times, most recently
from
40dde05
to
66a1b45
Compare
nicolo-ribaudo
added a commit
to nicolo-ribaudo/test262
that referenced
this pull request
Jan 30, 2024
nicolo-ribaudo
added a commit
to nicolo-ribaudo/test262
that referenced
this pull request
Jan 30, 2024
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
pushed a commit
to ptomato/test262
that referenced
this pull request
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. |
ptomato
force-pushed
the
duration-normalize-part-2
branch
from
66a1b45
to
006a2f9
Compare
ptomato
pushed a commit
to ptomato/test262
that referenced
this pull request
Jan 31, 2024
Ms2ger
approved these changes
Jan 31, 2024
ptomato
pushed a commit
to tc39/test262
that referenced
this pull request
Jan 31, 2024
This was referenced Jan 31, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(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.