Regression in rounding of Duration::from_secs_f64 in 1.60.0

[dtolnay]

use std::time::Duration;

fn main() {
    let dur = Duration::from_secs_f64(0.000042);
    assert_eq!(dur, Duration::new(0, 42000));
}

The above assertion succeeds on Rust standard library versions prior to 1.60.0. On 1.60.0 it fails with:

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `41.999µs`,
 right: `42µs`', src/main.rs:6:5

The exact mathematical value represented by 0.000042_f64 is:

0.0000419999999999999976759042230600726952616241760551929473876953125

which is about 41999.999999999998ns. This is astronomically closer to 42000ns than to 41999ns. I believe the correct behavior for from_secs_f64 would be to produce 42000ns as before, not truncate to 41999ns.

Mentioning @newpavlov @nagisa since #90247 seems related and is in the right commit range. The PR summary mentions that the PR would be performing truncation instead of rounding but I don't see that this decision was ever discussed by the library API team. (Mentioning @rust-lang/libs-api to comment on desired behavior.)

[nagisa]

See also #93535 (comment).

[dtolnay]

#93535 (comment) says that the decision in favor of truncating was motivated in part because the integer accessors on Duration, such as subsec_micros, do truncation not rounding. For example a duration with 41999 nanos would say it has 41 subsec micros, not 42.

I don't believe this rationale is correct to apply to from_secs_f64. People's expectations are quite different for integer manipulation vs floats. I think it is widely understood that / performs truncating division for integers and rounding for floats (mandated by IEEE 754).

Truncating is especially not a good behavior for from_secs_f64 because every float that is the closest f64 to an exact number of nanoseconds (like 0.000042_f64 is) is going to be in some tiny tiny interval around that number of nanoseconds, "randomly" either slightly smaller or slightly greater. It would be silly to split that interval and send the bottom half of it to a different result than the top half.

  exactly x-1 nanos          x nanos                    x+1 nanos
  |                          |                          |
 [ ]                        [ ]                        [ ]
 ^ ^
   float somewhere in this range

(In reality these intervals are 10000000000x smaller than shown in the ASCII art, which makes the truncating behavior even more absurd.)

None of the integer operations on Duration share a similar characteristic to this.

[m-ou-se]

Hm, some_int as f64 rounds, while some_float as i64 truncates. This is basically the some_float as i64 situation, which would suggest that truncating is the right option. Except that integers (whole numbers) are exactly representable, which makes the issue disappear:

    let mut x = 0.000042;
    println!("{x:.100}"); // 0.0000419999999999999976759042230600726952616241760551929473876953125000000000000000000000000000000000
    x *= 1e9;
    println!("{x:.100}"); // 42000.0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000

So, while 0.000042 is slightly less than it should be, (0.000042 * 1e9) as i64 still produces exactly 42000. Interestingly, the rounding that makes this work happens in the multiplication, not in the conversion to i64.

[m-ou-se]

That's not universal though. For example, (0.000000015 * 1e9) as i64 is 14, not 15.

And that already happened in Duration::from_secs_f64 before 1.60:

Latest Rust:

[src/main.rs:4] Duration::from_secs_f64(0.000000015).as_nanos() = 14
[src/main.rs:5] Duration::from_secs_f64(0.000042).as_nanos() = 41999

Rust 1.59.0:

[src/main.rs:4] Duration::from_secs_f64(0.000000015).as_nanos() = 14
[src/main.rs:5] Duration::from_secs_f64(0.000042).as_nanos() = 42000

[m-ou-se]

It might be reasonable to round to nanoseconds. (Simply add half a nanosecond to the f64 before converting it.) But the previous behaviour was also truncating, not rounding. It's just that rounding errors happen in slightly different places, making this a regression for 0.00042.

[m-ou-se]

Testing for every whole number of nanoseconds between 0 and 1, we had 'wrong' answers for 1.7% of them before the recent change (like 0.000000015s = 14ns), and for 48% of them after the change (like 0.00042s = 41999ns).

[m-ou-se]

Here's another argument for doing rounding, that does not involve floating point literals: It doesn't round-trip, even if f64 has more than enough precision:

[src/main.rs:7] Duration::from_secs_f64(Duration::from_nanos(15).as_secs_f64()).as_nanos() = 14

If we simply add 0.5e-9 to the f64 before converting it, to make it round instead of truncate, all these issues disappear.

[nagisa]

Simply add half a nanosecond to the f64 before converting it.

This is not something we can do. Taking the ASCII art given by dtolnay above, if we were to adopt a round-to-nearest scheme, we'd end up with something like this:

  exactly x-1 nanos         x nanos                x+1 nanos
          |                    |                      |
[                   ][                    ][                     ]
  round towards x-1      round towards x      round towards x+1

If we were to add half a nanosecond to the floating point value before truncating, we'd end up rounding wrong at the exact points where rounding… “direction” changes (as denoted by ][).

There are some descriptions on how to correctly implement round-to-nearest in software for example here, but it'll naturally make the algorithm that much more complicated.

---

It doesn't round-trip, even if f64 has more than enough precision

The roundtrip property does not and cannot hold for the values representable by Duration. Duration can hold 96 bits worth of data.

[newpavlov]

I've created the experimental PR which implements rounding. I am not 100% sure that rounding is implemented correctly and that I haven't missed any corner cases.

Personally, I think that having truncation in float-duration conversion is fine and that we should prefer it for code simplicity.

[dtolnay]

It doesn't round-trip, even if f64 has more than enough precision:

Duration::from_secs_f64(Duration::from_nanos(15).as_secs_f64()).as_nanos() = 14

As @nagisa wrote, the notion of round-trip you are using (duration -> f64 -> duration) is not something that can be lossless in general, so this doesn't work as an argument against truncating.

However there is a different round trip that would be valuable to support but broken by truncation: f64 -> duration -> f64 -> duration. Or in practical terms:

• You have a config file containing interval: 0.3 that you TOML deserialization / YAML deserialize using Serde into a Duration to do stuff with it, then rewrite the config file back out to disk with changes to some of the other fields

• or, You have a REST server that accepts {"interval":0.3} over HTTP, deserializes it from JSON using Serde into a Duration for validation, then re-encode to JSON and transmit forward to another server, which will deserialize and use the value.

The exact string representation of the float in the original incoming input is obviously not preserved because there could've been arbitrarily many irrelevant digits there, but a desirable property is that the first Duration and the second Duration have the same value.

Truncation breaks this because if 2 consecutive nanosecond quantities both truncate, then each time you deserialize+serialize, your data is gonna decrease by 1 nanosecond, potentially several times in the course of processing this request/whatever.

use std::time::Duration;

fn main() {
    let config_file = "0.00004205";
    let f = config_file.parse::<f64>().unwrap();
    let first_duration = Duration::from_secs_f64(f);
    let config_file = first_duration.as_secs_f64().to_string(); // 0.000042049
    let f = config_file.parse::<f64>().unwrap();
    let second_duration = Duration::from_secs_f64(f);

    assert_eq!(first_duration, second_duration); // desirable property
}

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `42.049µs`,
 right: `42.048µs`', src/main.rs:11:5

[dtolnay]

For me, ultimately I don't care so much about any of the "round trip" properties as @m-ou-se brought up. Whatever you end up doing with these duration values, std::thread::sleep or whatever, nobody is going to ever observe the physical effect being off by 1 nanosecond here or there.

The use case I do care about is logging. For example we have a config file containing interval: 0.7 that gets deserialized and printed to the user as our application does stuff. (I suspect config files are the dominant real-world use of from_secs_f64, maybe followed by cross-language data transmission like JSON APIs.) The user absolutely will notice if we print 699.999999ms, and will have a worse experience than if we manage to print their 0.7 accurately as 700ms.

[newpavlov]

The user absolutely will notice if we print 699.999999ms, and will have a worse experience than if we manage to print their 0.7 accurately as 700ms.

Even with rounding such cases are absolutely possible. For example, with both rounding and truncation 0.7s represented as f32 will be converted to 0 seconds and 699999988 nanoseconds.

[dtolnay]

I understand that f32 does not have 9 complete digits of sub-second precision, and neither does f64 for durations longer than tens of millions of seconds.

But f64 has way more than enough precision that we shouldn't be making these inaccuracies for all the ordinary durations that get used in the real world. f64 has enough precision that you can uniquely represent every duration up to 97 days to nanosecond precision. We really should not be messing this up in from_secs_f64 for values as small as 0.7 seconds.

[newpavlov]

Wouldn't it be better for cases like this to introduce a FromStr impl than to roundtrip through floats?

[dtolnay]

That's possible but I wouldn't consider it better than rounding in from_secs_f64. Currently there isn't a Display impl on Duration so I am skeptical that we could settle on a widely suitable FromStr impl. Especially consider that the Debug impl uses the Greek letter μ for "micro" which is not something you'd probably want to require someone to type in a config file.

[BurntSushi]

Popping up a level here, given that there was a change in behavior that wasn't explicitly discussed by libs-api, does it make sense to revert first and then discuss later whether the change should be made?

I understand it might be expedient to just try and have the discussion now, but if there's no immediately obvious consensus, then it might make sense to revert this behavior for now. (Unless I'm missing something and this behavior change was tied to something else that makes reverting difficult?)

[newpavlov]

@BurntSushi
The initially stabilized implementation was in a sense wrong, so #90247 is effectively a bug fix. I (the one who submitted both the initial implementation and #90247) strongly think that we either should merge #96051 or leave everything as-is.

@dtolnay

Especially consider that the Debug impl uses the Greek letter μ for "micro" which is not something you'd probably want to require someone to type in a config file.

us is a pretty common alternative to µs.

BTW, you've used a wrong character, the correct one is µ, i.e. U+B5, not U+03BC in the Greek and Coptic block. It's one of the "fun" Unicode peculiarities.

[joshtriplett]

Testing for every whole number of nanoseconds between 0 and 1, we had 'wrong' answers for 1.7% of them before the recent change (like 0.000000015s = 14ns), and for 48% of them after the change (like 0.00042s = 41999ns).

This seems like an extremely compelling argument.

[joshtriplett]

We discussed this in today's @rust-lang/libs meeting, and while we don't think this is anywhere near serious enough to warrant a stable backport, it's worth fixing to round again.

[joshtriplett]

#96051 (comment)

[thomcc]

BTW, you've used a wrong character, the correct one is µ, i.e. U+B5, not U+03BC in the Greek and Coptic block. It's one of the "fun" Unicode peculiarities.

Not that it's particularly important, this is not really accurate. That is, the character does exist, but according to https://www.unicode.org/reports/tr25/ U+03BC should be preferred, and the dedicated micro character is mostly present for compatibility:

Micro sign is included in several parts of ISO/IEC 8859, and therefore supported in many legacy environments where U+03BC μ GREEK LETTER SMALL MU is not available. Implementations therefore need to be able to recognize the micro sign, even though U+03BC μ is the preferred character in a Unicode context

[joshtriplett]

@thomcc In this context, compatibility and wider symbol availability would be a good thing. But I think that's off-topic for this issue.