New TimeSpan.From overloads which take integers

[tommysor]

Add new TimeSpan.From overloads as approved in #93890

The implementation is slightly inconsistent with existing methods.
This implementation will not overflow on intermediate calculations, and only throw if the final result is out of range.
I believe this inconsistency to be desirable based on comments on an earlier attempt (#95779)

For example, using 1067519900 days will result -1008547758080 microseconds which is going to be considered as a valid value while it is really overflowed.

Personally I think that we should only throw if the final tick count is out of range.
It's always annoying when x + y + z fails but x + z + y would succeed

fix #93890

[ghost]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-datetime
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Add new TimeSpan.From overloads as approved in #93890

The implementation is slightly inconsistent with existing methods.
This implementation will not overflow on intermediate calculations, and only throw if the final result is out of range.
I believe this inconsistency to be desirable based on comments on an earlier attempt (#95779)

For example, using 1067519900 days will result -1008547758080 microseconds which is going to be considered as a valid value while it is really overflowed.

Personally I think that we should only throw if the final tick count is out of range.
It's always annoying when x + y + z fails but x + z + y would succeed

fix #93890

Author:	tommysor
Assignees:	-
Labels:	new-api-needs-documentation, area-System.DateTime
Milestone:	-

[tannergooding]

This is definitely the simplest way to do this, but its also potentially more expensive than necessary for the common cases.

For a non-overflow case, what's the perf look like compared to doing (with today's double overloads) FromDays() + FromHours() + FromMinutes() + FromSeconds() + ...

[tommysor]

more expensive than necessary for the common cases.

Definitively, but its the only way I could see to allow the full range of inputs without overflowing (before the check of final result).
I believe the other 2 options would be to limit the range of inputs such that they are not able to overflow an Int64, or either accepting or checking for overflow.

The single argument overloads (like FromDays(int)) is, of course, trivial to optimize.

Unless I missed something it does not look like a chain of FromDays(double) + .. is the competitor though.
Its a bit quick and dirty (copy paste TimeSpan.cs and making it compile), but I think this should be right.
https://github.com/tommysor/TimeSpanMathPerf

• Doubles: FromDays(2.0) + .. + FromMicroseconds(7.0)
• Int128: FromDays(2, 3, 4, 5, 6, 7) (Implementation from this PR)
• Int64: new TimeSpan(2, 3, 4, 5, 6, 7) (same logic as Int128, but without the cast to Int128)

BenchmarkDotNet v0.13.12, Debian GNU/Linux 12 (bookworm) (container)
AMD EPYC 7763, 1 CPU, 2 logical cores and 1 physical core
.NET SDK 8.0.101
  [Host]     : .NET 8.0.1 (8.0.123.58001), X64 RyuJIT AVX2
  DefaultJob : .NET 8.0.1 (8.0.123.58001), X64 RyuJIT AVX2

Method	Mean	Error	StdDev	Median	Ratio	RatioSD
Doubles	13.8305 ns	0.4129 ns	1.2045 ns	13.4968 ns	1.000	0.00
Int128	6.9789 ns	0.1914 ns	0.5553 ns	6.8646 ns	0.508	0.06
Int64	0.0283 ns	0.0198 ns	0.0478 ns	0.0000 ns	0.002	0.00

[tannergooding]

new TimeSpan(2, 3, 4, 5, 6, 7)

Note that you may want to test this with something the JIT can't see as constant. It's fully folding it away and so isn't a great comparison.

---

The basic here is that we're already 2x as fast as what users can get today (if they want to guarantee accuracy) via double, but we're likely still quite a bit slower than we could be and the current code can't be constant folded. The multiplications that Int128 is doing are also more expensive than needed by quite a bit since we know both inputs are within 64-bits.

We can't really do much without the JIT recognizing BigMul as constant foldable. However, you can likely make it a little cheaper with the following:

public static TimeSpan FromDays(int days, int hours = 0, long minutes = 0, long seconds = 0, long milliseconds = 0, long microseconds = 0)
{
    Int128 totalMicroseconds = Math.BigMul(days, MicrosecondsPerDay)
                             + Math.BigMul(hours, MicrosecondsPerHour)
                             + Math.BigMul(minutes, MicrosecondsPerMinute)
                             + Math.BigMul(seconds, MicrosecondsPerSecond)
                             + Math.BigMul(milliseconds, MicrosecondsPerMillisecond)
                             + microseconds;

    long lowerMicroseconds = totalMicroseconds.Lower;
    long upperMicroseconds = totalMicroseconds.Upper;

    if (((upperMicroseconds == 0) && ((ulong)lowerMicroseconds > MaxMicroseconds)) || (upperMicroseconds != -1) || ((ulong)lowerMicroseconds < (ulong)MinMicroseconds))
    {
        ThrowHelper.ThrowArgumentOutOfRange_TimeSpanTooLong();
    }
    var ticks = (long)totalMicroseconds * TicksPerMicrosecond;
    return TimeSpan.FromTicks(ticks);
}

// In Math.cs
internal static Int128 BigMul(long a, long b)
{
    long upper = BigMul(a, b, out long lower);
    return new Int128((ulong)upper, (ulong)lower);
}

This should do less work for the multiplications and keeps it very cheap for the additions. I expect doing checked addition at each step would be overall more expensive than what Int128 is doing and we know it can't overflow Int128 already. We can then make it cheaper over time by recognizing a couple interesting patterns in the JIT (ones we want to recognize long term anyways).

[tarekgh]

(upperMicroseconds != -1)

@tannergooding Is this for checking the negative sign? I mean the upperMicroseconds == -1 means the int128 is negative. right?

[tannergooding]

Correct, which is allowed for TimeSpan, just only if its within [MinMicroseconds, MaxMicroseconds]

The upperMicroseconds == 0 means that its a small positive (fits into 64-bits). If its a small positive and greater than MaxMicroseconds, we should throw. Here we check by comparing lowerMicroseconds as unsigned since we want the most significant bit being set to throw.

The upperMicrosedconds != -1 then means its either a large positive or a large negative value (takes more than 64-bits), in which case we should throw.

Otherwise it's a small negative (fits into 64-bits) and we should throw if the lower Microseconds is less than MinMicroseconds. In this case, we again check by comparing the lowerMicroseconds as unsigned, because we're ultimately wanting it to be between [-1, MinMicroseconds] which in hex is [0xFFFF_FFFF_FFFF_FFFF, 0xF333_3333_3333_3334], so we want the most significant bit set and the value to be greater than or equal to MinMicroseconds in unsigned form

[tommysor]

fully folding it away

wops, no wonder the Int64 was so ridiculously fast.

And yes, seems like BigMul is a little cheaper (not tested the change to overflow check since I don't have access to internal totalMicroseconds.Lower in that benchmark repo)

https://github.com/tommysor/TimeSpanMathPerf

• Doubles: FromDays(_daysDouble) + .. + FromMicroseconds(_microsecondsDouble)
• Int128: FromDays(_daysInt, .., .., .., .., _microsecondsInt) (Implementation from this PR)
• Int128_BigMul: Using BigMul for multiplication
  Still same overflow check since my copied code does not have access to the internal totalMicroseconds.Lower
• Int64: new TimeSpan(_daysInt, .., .., .., .., _microsecondsInt) (same logic as Int128, but without the cast to Int128)

BenchmarkDotNet v0.13.12, Debian GNU/Linux 12 (bookworm) (container)
AMD EPYC 7763, 1 CPU, 2 logical cores and 1 physical core
.NET SDK 8.0.101
  [Host]     : .NET 8.0.1 (8.0.123.58001), X64 RyuJIT AVX2
  DefaultJob : .NET 8.0.1 (8.0.123.58001), X64 RyuJIT AVX2

Method	Mean	Error	StdDev	Median	Ratio	RatioSD
Doubles	15.9139 ns	0.3832 ns	1.0871 ns	15.4181 ns	1.00	0.00
Int128	7.3156 ns	0.1990 ns	0.5835 ns	7.0550 ns	0.46	0.05
Int128_BigMul	6.4639 ns	0.2071 ns	0.6041 ns	6.2065 ns	0.41	0.05
Int64	1.7092 ns	0.0677 ns	0.1698 ns	1.6955 ns	0.11	0.01

[tarekgh]

Looks BigMul is the way to go. I am not concerning about the overflowing case performance as I am not expecting this will occur much with this APIs. @tommysor thanks for getting this numbers. Could you please update the code so we can move forward with this PR?

[tarekgh]

if ((totalMicroseconds > MaxMicroseconds) || (totalMicroseconds < MinMicroseconds))

I think this is correct, I am just wondering why @tannergooding suggested doing like

if (((upperMicroseconds == 0) && ((ulong)lowerMicroseconds > MaxMicroseconds)) || (upperMicroseconds != -1) || ((ulong)lowerMicroseconds < (ulong)MinMicroseconds))

[tommysor]

I kept the original comparison here because it works.
When I tried to simply copy Tanners suggestion it would not build, and my attempts at corrections made the tests fail.
To go further with that I think I would have to spend some time playing with Int128, long and ulong to get from "understand the general idea of what Tanner is trying to do" to actually "understand how to make that work".

I assume Tanner suggested this for performance, though I don't know how this helps.
I'm guessing comparison to Int128 has some overhead that may make it worth doing more and more complicated comparisons.

I also tried to use dotnet/performance repo to benchmark the actual implementation, but I could not get the benchmark to find the new overloads.
If I understood the docs right, that should be a lot easier shortly after they are merged.

PS: I expect to do a follow up PR to do the trivial optimizations of the single argument methods.

[tarekgh]

Modulo the minor comments I left, LGTM. Thanks @tommysor for your help with it.

@tannergooding do you want to take one last look before merging?

[tannergooding]

It might be worth optimizing these ones (that take a single arg) directly rather than deferring them down. There's quite a bit of code that can be skipped and they could all use a single shared helper that looks like:

public static TimeSpan FromDays(int days) => FromUnit(days, TicksPerDay, MinDays, MaxDays);

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static TimeSpan FromUnit(int unit, long ticksPerUnit, long minUnits, long maxUnits)
{
    Debug.Assert(minUnits < 0);
    Debug.Assert(maxUnits > 0);

    if ((unit > maxUnits) || (unit < minUnits))
    {
        ThrowHelper.ThrowArgumentOutOfRange_TimeSpanTooLong();
    }
    return TimeSpan.FromTicks(units * TicksPerUnit);
}

We could also just replicate the basically 3 lines of code in each case, it's not a lot. No preference on my end.

Note that I don't think it needs to be done in this PR, but its certainly easier to get in if we did it here.

[tannergooding]

Same general note for these ones. We have a few lines of code to replicate that would allow completely skipping more expensive parts of the FromDays algorithm, the savings increase the lower the unit of time we have is.

It's a bit more code, but given these are in part there for perf and accuracy, I personally think that's fine. It might be different if we could constant fold Int128 arithmetic, but that's not something the JIT is likely to get in the .NET 9 timeframe.

[tannergooding]

Actual logic looks good/correct to me.

Left a small suggestion around duplicating some of the code due to known JIT limitations, but I don't think that's a blocker or requirement

[tarekgh]

@tommysor is it possible you can apply @tannergooding suggestion before merge? I prefer that if possible as we don't have to revisit it again. Thanks!

[tommysor]

is it possible you can apply @tannergooding suggestion before merge?

@tarekgh yes. I have added in the optimizations of the single argument methods, and Tanners suggested optimization on 1 of the multi argument methods (FromMilliseconds).
I can do the others as well, but not today (I'm out of time for tonight).

[tarekgh]

@tommysor thanks again. no rush, we can wait to get the remaining optimization.