TimeSpan.FromMilliseconds produces inconsistent results

[kolixx]

According to the documentation, there should be exactly 10000 ticks in a millisecond: https://docs.microsoft.com/en-us/dotnet/api/system.datetime.ticks?view=net-5.0

However, this is not true. If you create time spans based on a large number of milliseconds, this invariant fails. Here are two examples:

• TimeSpan.FromMilliseconds (315537897599999).Ticks returns 3155378975999989760 instead of 3155378975999990000
• TimeSpan.FromMilliseconds(315537897599997).Ticks returns 3155378975999969792 instead of 3155378975999970000

If you decrease the order of magnitude of milliseconds by two or three orders, then the results are good. The number of ticks will be exactly 10000 times the number of milliseconds.

[dotnet-issue-labeler[bot]]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[danmoseley]

This is because the multiplication is done in a double and then cast back to long and these particular values have no round trippable representation in a double.

https://github.com/dotnet/runtime/blob/41f3d48b520d4cf4f9e051dc07f480dcb9c62e08/src/libraries/System.Private.CoreLib/src/System/TimeSpan.cs#L202

This could probably be improved, but I'm curious do you have a real need for a 10,000 year TimeSpan?

[ghost]

Tagging subscribers to this area: @tannergooding See info in area-owners.md if you want to be subscribed.

Issue Details

---

According to the documentation, there should be exactly 10000 ticks in a millisecond: https://docs.microsoft.com/en-us/dotnet/api/system.datetime.ticks?view=net-5.0 However, this is not true. If you create time spans based on a large number of milliseconds, this invariant fails. Here are two examples: - TimeSpan.FromMilliseconds (315537897599999).Ticks returns 3155378975999989760 instead of 3155378975999990000 - TimeSpan.FromMilliseconds(315537897599997).Ticks returns 3155378975999969792 instead of 3155378975999970000 If you decrease the order of magnitude of milliseconds by two or three orders, then the results are good. The number of ticks will be exactly 10000 times the number of milliseconds.

Author:	kolixx
Assignees:	-
Labels:	`area-System.Runtime`, `untriaged`
Milestone:	-

[danmoseley]

Cc @kolixx in case subscribers don't follow issue moves

[kolixx]

This could probably be improved, but I'm curious do you have a real need for a 10,000 year TimeSpan?

@danmoseley I gave examples that came from a long and painful bug fixing investigation so we did definitely stumble upon such examples. We went around the issue by using different constructors for the TimeSpans, and now we have automated checks in place to prevent people from constructing TimeSpans in ways that might cause such inconsistencies. So in this respect we are fine.

My preferred solution would be to check if a certain construction would introduce numerical errors, and then not allow that construction to go through. Very much in the same spirit of the check here

https://github.com/dotnet/runtime/blob/41f3d48b520d4cf4f9e051dc07f480dcb9c62e08/src/libraries/System.Private.CoreLib/src/System/TimeSpan.cs#L208

It would make detecting such issues much easier and the vast majority of people not creating such long TimeSpans would not see any change.

[kolixx]

@danmoseley From the other direction, I would not expect that the code is changed such that larger intervals remain consistent. Only if a certain construction did succeed, then it should behave according to specifications.

[danmoseley]

@tannergooding is an expert on numerical precision and perhaps can recommend a change. It seems like we could avoid double when we are dealing with long. But I'll let him comment

[tannergooding]

For better or for worse, the public surface area of many APIs on TimeSpan take and return double. This means that any value input or returned that is greater than 2^52 is guaranteed to have a loss in precision and any fractional portion is likely to be lossy (not be exactly representative).

We could improve the handling in Interval, but it will likely come at the cost of some perf. The "simplest" fix here, which will help with whole integrals between 2^52 and 2^63, is to change private static TimeSpan Interval(double value, double scale) to private static TimeSpan Interval(double value, long scale) and then update it to handle the fractional and integer scaling separately:

private static TimeSpan Interval(double value, long scale)
{
    if ((value > long.MaxValue) || (value < long.MinValue) || || double.IsNaN(value)
        throw new OverflowException(SR.Overflow_TimeSpanTooLong);

    double integer;
    double fractional = Math.ModF(value, &integer);

    long ticks = checked(((long)integer * scale) + (long)(fractional * scale));
    return (ticks == long.MaxValue) ? TimeSpan.MaxValue : new TimeSpan((long)ticks);
}

Noting that I have done no testing on the above and so its likely there is a bug or something I'm not remembering having just woken up 😄