Improve precision of Duration-float operations

[eggyal]

View all comments

Rather than convert Duration to a float in order to multiply or divide by a floating point number, which entails a loss of precision, we instead represent both operands as u128 nanoseconds and perform integer arithmetic thereupon.

This improvement in precision affects some of the documented examples.

Given that these methods have been stabilised, is it a breaking change to affect their output? If so, this PR obviously cannot be merged as-is: we might instead need to create new methods for this more precise calculation (and possibly deprecate the existing ones).

Fixes #149794
r? libs-api

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[eggyal]

@rustbot ready

[eggyal]

No, some corner cases are not quite right here. I'll add some more tests and push a fix.

@rustbot author

[eggyal]

@rustbot ready

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[tgross35]

I don't expect it to tell us that much, but I think this works enough to kick off a crater run.

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 8da2082 (8da20828b6ea601dfd8e2448cb50ace88876bd78, parent: d933cf483edf1605142ac6899ff32536c0ad8b22)

[tgross35]

@craterbot run mode=build-and-test

[craterbot]

👌 Experiment pr-150933 created and queued.
🤖 Automatically detected try build 8da2082
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[eggyal]

Should we run crater while our own tests are failing? Surely I should fix that first!

[tgross35]

The failure is just a changed panic string right? I don't expect anyone to be checking that

[eggyal]

We also were introducing new panics for some negative factors that previously yielded zero, which has been corrected in my latest push.

@rustbot ready

[Amanieu]

Once you get into floating-point arithmetic, some amount of precision loss is to be expected. I actually don't find it too surprising that multiplying by 1.0 would result in precision loss for very large durations. The main problem with the code in this PR is that it adds a huge amount of complexity for what is really an edge case. It would be more appropriate to just have a warning in the documentation which explains that this method may cause precision loss due to the use of floating-point arithmetic, even when multiplying with 1.0.

@rfcbot close libs

[rust-rfcbot]

Team member @Amanieu has proposed to close this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @cuviper
• @joshtriplett
• @the8472
• @thomcc

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.