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

Implementer concern: When can subdurations lose precision? #2195

Closed
rkirsling opened this issue May 12, 2022 · 6 comments
Closed

Implementer concern: When can subdurations lose precision? #2195

rkirsling opened this issue May 12, 2022 · 6 comments
Assignees
Labels
normative Would be a normative change to the proposal spec-text Specification text involved
Milestone

Comments

@rkirsling
Copy link
Member

rkirsling commented May 12, 2022

While I'm pleased about #2094's specifying that Duration fields be f64-representable, this test raises a significant practical concern about "who should be responsible for not losing precision".

In particular:

  1. If the difference in nanoseconds between two Instants is not f64-representable, it is actually reasonable to expect that the BalanceDuration call in DifferenceTemporalInstant not be lossy?
  2. Preventing loss in this case requires that the implementation of TotalDurationNanoseconds not be an f64 operation. This is a tricky line to walk:
    • The general case of adding two Duration fields must be an f64 operation, otherwise what have we gained by storing them as such?
    • It may still be logical to say that "summing all the fields together for rebalancing should not be subject to loss during the calculation". This just gets a bit funky in RoundDuration steps 6 and 7, where rounding to a date unit uses TotalDurationNanoseconds but rounding to a time unit does some direct (non-integral) addition.
@ptomato
Copy link
Collaborator

ptomato commented May 13, 2022

Thanks, this is good feedback. We'll discuss it in our next champions meeting. You are welcome to join in, but in the meantime do you have any potential solutions in mind? (e.g. do the balance arithmetic in the f64 domain)

@rkirsling
Copy link
Member Author

Thanks!

At first I was thinking, "could we just 'fix' the test?", but untested does not mean un(der)specified, so that's a cop-out.

Perhaps a concrete question to focus on here is:
"Should duration.toString() === duration.round('nanoseconds').toString() be an invariant?"

This is nice because it doesn't involve crossing in/out of the world of durations. And we might want to say yes, when considering PT1111111011.111111101S (the duration in the test), where precision loss only occurs at the point of expressing the total duration in nanoseconds. But then subdurations needn't even fit into Int128s—m = 2**129; new Duration(0,0,0,m,m,m,m,m,m,m) still has a finite total—so of course we're forced to say no.

If durations being unbounded implies that TotalDurationNanoseconds cannot avoid precision loss (or integer overflow, which presumably would be worse), then it seems hard to justify DifferenceTemporalInstant as a special case purely because it's crossing into the world of durations anew. Realistically, it's natural for the thing being balanced in BalanceDuration to be an array of doubles.

@justingrant
Copy link
Collaborator

To close the loop, there was previous discussion (about putting bounds on the size of Duration fields) that may be relevant to this issue. IMO, the most relevant part of that discussion probably starts with @rkirsling's comment but continues down quite a bit:

Since there doesn't seem to be a precedent for printing the digits of a Number value larger than 1021 (the magic number in Number.p.toFixed), this seems like a pretty significant topic in need of discussion.

@rkirsling
Copy link
Member Author

rkirsling commented Jul 28, 2022

Importantly, balancing logic also occurs in steps 2-7 of TemporalDurationToString. This is an even worse situation than DifferenceTemporalInstant because it goes beyond f64-representability into f64-storability.

Namely, the spec currently suggests that Temporal.Duration.from({ microseconds: Number.MAX_VALUE, nanoseconds: Number.MAX_VALUE }).toString() should somehow manage to display seconds as a decimal number, even though this is Infinity in the f64 domain. It's unfortunate enough to need to use BigInt#toString instead of Number#toString to display numbers beyond Number.MAX_SAFE_INTEGER, but repeating what I said in the issue description, if we're not even allowed to add two subdurations in the f64 domain, then we've gained nothing by storing f64s. I'll plan to implement this particular case as a RangeError for now.

webkit-early-warning-system pushed a commit to rkirsling/WebKit that referenced this issue Jul 29, 2022
…nd MAX_SAFE_INTEGER

https://bugs.webkit.org/show_bug.cgi?id=243323

Reviewed by Yusuke Suzuki.

Duration fields are currently specified as finite integral Numbers; unless a further bound is introduced in the future,
this means we'll need to use BigInt#toString for values beyond MAX_SAFE_INTEGER. (Until now we've just being using 1e21
as a maximum, since this is the cutoff for Number#toFixed.) This is somewhat unfortunate since the use of BigInt incurs
a bunch of new exception logic.

Furthermore, TemporalDurationToString somehow expects us to display seconds as a decimal number in a case like
`Temporal.Duration.from({ microseconds: Number.MAX_VALUE, nanoseconds: Number.MAX_VALUE }).toString()`,
where calculating in the Number domain produces Infinity. I've added this to my concerns in tc39/proposal-temporal#2195;
for now, I think it makes the most sense to implement this as a RangeError.

* JSTests/test262/expectations.yaml:
* Source/JavaScriptCore/runtime/TemporalDuration.cpp:
(JSC::TemporalDuration::toString const):
(JSC::appendInteger):
(JSC::TemporalDuration::toString):
* Source/JavaScriptCore/runtime/TemporalDuration.h:
* Source/JavaScriptCore/runtime/TemporalDurationPrototype.cpp:
(JSC::JSC_DEFINE_HOST_FUNCTION):

Canonical link: https://commits.webkit.org/252935@main
@ptomato ptomato added the normative Would be a normative change to the proposal label Sep 28, 2022
@ptomato ptomato added this to the Stage "3.5" milestone Dec 8, 2022
@ptomato ptomato added spec-text Specification text involved and removed meeting-agenda labels Jan 3, 2023
@ptomato
Copy link
Collaborator

ptomato commented Jan 16, 2024

Addressed thoroughly by #2727.

@ptomato ptomato self-assigned this Jan 16, 2024
@ptomato
Copy link
Collaborator

ptomato commented Feb 1, 2024

We believe this is addressed now. Duration units are specified to have upper bounds such that it should be possible to do all calculations using 84-bits of integer math (e.g., i128 or i64+i32) or two f64s.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative Would be a normative change to the proposal spec-text Specification text involved
Projects
None yet
Development

No branches or pull requests

3 participants