Duration parsing loses precision by using Double when Long would work

[Blaisorblade]

@melrief pointed out on Gitter that pretty-printing and parsing a Duration does not round-trip in some cases, even for Durations that can be perfectly represented as Long. The code reads the number as a Double and then rounds it to a Long. Not clever:
https://gitter.im/scala/scala?at=5919588a2b926f8a675791b0

scala> Duration("6803536004516701ns")
res2: scala.concurrent.duration.Duration = 6803536004516702 nanoseconds

I'm looking at his reports, details upcoming.

[Blaisorblade]

@melrief pointed to two problematic code locations. Took me a while to agree, but he's right.

1. fromNanos(Double) uses (nanos + 0.5).toLong, presumably to round nanos (https://github.com/scala/scala/blob/2787b47396013a44072fa7321482103b66fbccd3/src/library/scala/concurrent/duration/Duration.scala#L128). But that doesn't work on 6803536004516701.0, while using .round would:

scala> val d = 6803536004516701.0
d: Double = 6.803536004516701E15

scala> d.round
res25: Long = 6803536004516701

scala> (d+0.5).toLong
res26: Long = 6803536004516702

In general, that doesn't work on numbers that are odd and use all available digits of Duration, as 6803536004516701.0 (see also Duration.maxPreciseDouble in the source).

In binary arithmetic, that behavior seems plausible, because at the least-significant digits you have 0.1_2 + 0.01_2 = 0.11_2 ~= 1 while 0.0_2 + 0.01_2 = 0.01_2 ~= 0_2.

2. Parsing code picks Doubles over Longs as long as the parsed value fits exactly in a Double in https://github.com/scala/scala/blob/2787b47396013a44072fa7321482103b66fbccd3/src/library/scala/concurrent/duration/Duration.scala#L60-L65. The resulting Double must then be processed into a Long anyway. That seems pretty fragile—but overall the code works on maxPreciseDouble:

scala> Duration("9007199254740992ns")
res1: scala.concurrent.duration.Duration = 9007199254740992 nanoseconds

[leifwickland]

@Blaisorblade I don't follow why you conclude that "overall the code works on maxPreciseDouble. For values just below maxPreciseDouble, i.e. 2^53, I see a loss of precision. I don't see a loss of precision near 2^52, which is what we'd expect based on the definition of a Double.

scala> import scala.concurrent.duration._
import scala.concurrent.duration._

scala> val twoTo53: Long = 1L<<53
twoTo53: Long = 9007199254740992

scala> for (d <- (0 to 10)) { val i = twoTo53 - d; val s = s"$i ns"; println(Duration(s)) }
9007199254740992 nanoseconds
9007199254740992 nanoseconds
9007199254740990 nanoseconds
9007199254740990 nanoseconds
9007199254740988 nanoseconds
9007199254740988 nanoseconds
9007199254740986 nanoseconds
9007199254740986 nanoseconds
9007199254740984 nanoseconds
9007199254740984 nanoseconds
9007199254740982 nanoseconds

scala> val twoTo52: Long = 1L<<52
twoTo52: Long = 4503599627370496

scala> for (d <- (0 to 10)) { val i = twoTo52 - d; val s = s"$i ns"; println(Duration(s)) }
4503599627370496 nanoseconds
4503599627370495 nanoseconds
4503599627370494 nanoseconds
4503599627370493 nanoseconds
4503599627370492 nanoseconds
4503599627370491 nanoseconds
4503599627370490 nanoseconds
4503599627370489 nanoseconds
4503599627370488 nanoseconds
4503599627370487 nanoseconds
4503599627370486 nanoseconds

[Blaisorblade]

I don't follow why you conclude that "overall the code works on maxPreciseDouble.

That statement is technically true on maxPreciseDouble (Duration("${1L<<53}ns") works, as confirmed by your tests), but a bad summary. So let me try again.

I'm questioning both (1) the rounding and (2) parsing into Double, but it seems this issue is only due to (1). That can be confirmed or falsified by testing scala/scala#5913.
I don't have a smoking gun against (2); I still don't like it as explained, but it's not clear how to do better.

For values just below maxPreciseDouble, i.e. 2^53, I see a loss of precision.

As explained by rounding. Doubles up to maxPreciseDouble can be represented exactly, they're just tricky to round:

scala> (1L << 53).toDouble.toLong == (1L << 53)
res3: Boolean = true
scala> (-1L << 53).toDouble.toLong == (-1L << 53)
res7: Boolean = true

[Jasper-M]

I think this is fixed by scala/scala#5796 ? Or is this also still about using Double for parsing?

[Blaisorblade]

Or is this also still about using Double for parsing?

I remember that seemed dodgy, but I'm not 100% sure how hard it is to change it and if it's better. If you think it's acceptable, feel free to close—not sure I can look into it (and I'm certain I can't look at it soon).

[SethTisue]

closing based on the last two comments, but someone might like to double check it