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

[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.

[craterbot]

🚧 Experiment pr-150933 is now running

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

[craterbot]

🎉 Experiment pr-150933 is completed!
📊 358 regressed and 300 fixed (837236 total)
📊 3182 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-150933/retry-regressed-list.txt ↩

[quaternic]

@Amanieu

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.

I agree on the complexity, but I do still think the issue needs fixing. The current implementation doesn't follow the basic principle of floating point arithmetic: Compute the exact result and round to the output type. It doesn't make sense that Duration can represent aeons with nanosecond precision, but the rounding error of these operations can be thousands of years. Duration::MAX.div_f32(3.0) and Duration::MAX / 3 differ by ~5800 years.

The complexity of this PR is unfortunate, but it's mostly due to the ad hoc implementation of scaling an integer by a float. It could be significantly cleaned up with a hypothetical u128::checked_mul_f64(u128, f64) -> Option<u128>. In fact, f128::mul could be used for this as well; map the count of nanoseconds to the subnormal range of f128, do floating point multiplication (which does the proper rounding), and map back to an integer with a range check.

[cuviper]

If we want to keep the full Duration precision, then I think f128 is a much better way to manage the complexity -- see #154107.