dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.19k stars 4.72k forks source link

DateTime.AddSeconds(double) precision error #80549

Open StefanBertels opened 1 year ago

StefanBertels commented 1 year ago

Description

Upgrading to .NET 7.0 results in (small) DateTime errors when adding seconds (double), probably introduced by https://github.com/dotnet/runtime/pull/73198.

While it's clear that this cannot be precise for every value it should be precise for values within some range (including the attached example which works flawless in .NET 6.0).

Reproduction Steps

[Fact]
void DateTimeAddSecondsRegression()
{
    var dt = DateTime.Parse("2012-09-08T01:22:27.249");

    Assert.Equal(dt, dt.Date.AddTicks(Convert.ToInt64(dt.TimeOfDay.TotalSeconds * TimeSpan.TicksPerSecond)));
    Assert.Equal(dt, dt.Date.AddSeconds(dt.TimeOfDay.TotalSeconds)); // fails in .NET7
}

Expected behavior

Test should be successful every time.

Actual behavior

Test fails for .NET 7.0 (while successful for .NET 6.0).

Regression?

Yes

Known Workarounds

Workaround: Avoid AddSeconds() method and use your own calculation using ticks.

Configuration

Other information

https://github.com/dotnet/runtime/blob/2335269/src/libraries/System.Private.CoreLib/src/System/DateTime.cs#L862

Joe4evr commented 1 year ago

According to #66815 it's the .NET 6 behavior that was considered the bug.

tannergooding commented 1 year ago

Right. The root issue here is you're expecting double to work like decimal. Where-as double is a binary-based floating-point type and can only accurately represent numbers that are multiples of powers of 2 and within the precision range of the type.

In the case of dt.TimeOfDay, the Ticks is an integer value of 49_472_490_000. TicksPerSecond is also an exact integral of 10_000_000. However, TotalSeconds returns a double and since 4947.249 is not exactly representable you actually get the nearest representable value which is 4947.24899999999979627318680286407470703125 (this displays in ToString() as 4947.249 but you can get the full precision by using ToString("G42") in this case).

This initial imprecision means that when you call AddSeconds you're not actually asking for 49472490000 ticks to be added, but for 49472489999.9999979627318680286407470703125 ticks to be added.

Correctly handling all the edge cases so that the fractional ticks in the "infinitely precise result" given the "exact represented input" (that is the 0.9999979627318680286407470703125) are allowed to round the final result up (from 49472489999 to 49472490000) is quite complex and expensive. Doing so would add a non-trivial amount of overhead to these APIs and significantly decrease performance. There are much cheaper "hacks" that could make this particular case work, but it'd be trading this "off by one" for a different set of "off by one" and it still wouldn't solve the scenarios where the requested Add*(someDouble) isn't doing what the user expected because of how double represents values.

-- For reference the "hack" I mentioned above would be changing the implementation from:

private DateTime AddUnits(double value, long maxUnitCount, long ticksPerUnit)
{
    if (Math.Abs(value) > maxUnitCount)
    {
        ThrowAddOutOfRange();
    }

    double integralPart = Math.Truncate(value);
    double fractionalPart = value - integralPart;
    long ticks = (long)(integralPart) * ticksPerUnit;
    ticks += (long)(fractionalPart * ticksPerUnit);

    return AddTicks(ticks);
}

To something like:

private DateTime AddUnits(double value, long maxUnitCount, long ticksPerUnit)
{
    if (Math.Abs(value) > maxUnitCount)
    {
        ThrowAddOutOfRange();
    }

    double integralPart = Math.Truncate(value);
    double fractionalPart = value - integralPart;
    long ticks = (long)(integralPart) * ticksPerUnit;
    ticks += (long)(Math.Round(fractionalPart * ticksPerUnit));    // Alternatively, for older hardware: `fractionalPart *= ticksPerUnit; ticks += (fractionalPart >= 0.5) ? 1 : 0`

    return AddTicks(ticks);
}

But, this would still be roughly 8 cycles more expensive per operation (about 30% slower) and would have various other edge cases that would still be incorrect.

StefanBertels commented 1 year ago

Of course precision errors for some (more extreme) double values is inevitable when converting to long (and vice versa). But I think math rules should apply here -- including round.

This value fails, too:

DateTime.Parse("2023-01-12T00:00:01.2Z")

I don't know what implementation of rounding is the fastest one, but I don't think this is really ok.

tannergooding commented 1 year ago

As mentioned, the cheapest "fix" here will make the Add*() methods (except AddTicks) around 30% slower and still won't be correct for all cases.

This is a considerable cost increase and it is ultimately up to the area owners (CC. @tarekgh) on if that is acceptable.

Neme12 commented 1 year ago

Right. The root issue here is you're expecting double to work like decimal.

I agree. But then why isn't there an overload of TimeSpan.FromSeconds that takes a decimal?

Neme12 commented 1 year ago

Oh, I see the issue here is that all of the TimeSpan.Total* properties return a double, which can't be changed to decimal :(