Implements new intand long overloads for TimeSpan

[tcortega]

I went ahead and took the liberty to add an public static System.TimeSpan FromMilliseconds(long milliseconds); that was not detailed in the proposal, but felt like it does not make sense to not add it.

[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.

[tcortega]

@dotnet-policy-service agree

[tcortega]

Resolves #93890

[ghost]

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

Issue Details

---

I went ahead and took the liberty to add an public static System.TimeSpan FromMilliseconds(long milliseconds); that was not detailed in the proposal, but felt like it does not make sense to not add it.

Author:	tcortega
Assignees:	-
Labels:	new-api-needs-documentation, community-contribution, area-System.DateTime, needs-area-label
Milestone:	-

[danmoseley]

I went ahead and took the liberty to add an public static System.TimeSpan FromMilliseconds(long milliseconds); that was not detailed in the proposal, but felt like it does not make sense to not add it

It will need API review if it wasn't approved... that may be quick.

[tarekgh]

public static TimeSpan FromDays(int days, int hours = 0, long minutes = 0, long seconds = 0, long milliseconds = 0, long microseconds = 0)

Please look at the code of the constructor public TimeSpan(int days, int hours, int minutes, int seconds, int milliseconds, int microseconds). It has some logic to ensure no overflow by checking the total max microseconds. I suggest you refactor this code into its own method and then call it from here and from the constructor.

[tarekgh]

wait, we are using long and not int for the parameters. I think we need to implement this to ensure there is no overflow will occur. will not be able to share the code with the constructor :-(

[tcortega]

@tarekgh I'm struggling to see the benefit of manually checking for overflows given that long.MaxValue represents the upper limit for TimeSpan's length. Any computation exceeding this limit inherently triggers an OverflowException. Implementing explicit checks seems to introduce additional CPU overhead, thus potentially reducing performance.

Here are some benchmarks:

| Method                            | Input               | Mean          | Error      | StdDev     | Gen0   | Allocated |
|---------------------------------- |-------------------- |--------------:|-----------:|-----------:|-------:|----------:|
| DefaultOverflowException          | 9223372036854775807 |     0.0192 ns |  0.0177 ns |  0.0165 ns |      - |         - |
| ManualOverflowChecksThenException | 9223372036854775807 | 3,188.9095 ns | 42.1336 ns | 39.4118 ns | 0.0229 |     240 B |

[tarekgh]

Any computation exceeding this limit inherently triggers an OverflowException

No. this will trigger it if the operation is checked which we don't enable by default. https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/checked-and-unchecked

[tcortega]

Any computation exceeding this limit inherently triggers an OverflowException

No. this will trigger it if the operation is checked which we don't enable by default. https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/checked-and-unchecked

I see, and what are the implications of having the checked keyword being used explicitly in there?
I re-did the benchmarks and seems like using the checked context is in fact slower than manually checking for overflows.

[tarekgh]

That is what I was trying to point at in my comment #94143 (comment)

[tarekgh]

TimeSpan

all introduced method need to ensure no overflow and and not exceeding the max allowed value.

[tcortega]

Waiting for your reply in the review up there

[tarekgh]

replied. We can try to optimize the checking as much as we can but have to ensure we produce the correct behavior.

[tarekgh]

enable checked still have some perf cost too and we'll need to decide if we need to catch the thrown overflow exception and convert it to something like ThrowHelper.ThrowArgumentOutOfRange_TimeSpanTooLong(). @bartonjs do you know if we have any design guidelines around using checked in the APIs? I did some search but couldn't find any.

[danmoseley]

@tannergooding do you see any trick to efficiently detect overflow for this particular calculation? Something better than checking whether the sign changed after each multiplication, or doing it all in an int128

[tcortega]

The CheckedKeyword results also look decent. Considering exceptions should be rare, the extra 10 microseconds and 8 bytes might not be a big deal.

[tannergooding]

I’d just go with the simple approach I detailed above as it lends the most capability for the JIT to optimize it.

We can revisit and optimize further if required later. Some of the easiest optimizations will be low hanging fruit such as improving operator +

[tannergooding]

That is, I'd go with doing this:

public static TimeSpan FromHours(int hours, long minutes = 0, long seconds = 0, long milliseconds = 0, long microseconds = 0)
{
    return FromHours(hours)
         + FromMinutes(minutes)
         + FromSeconds(seconds)
         + FromMilliseconds(milliseconds)
         + FromMicroseconds(microseconds);
}

The other patterns will end up with less capability for the JIT to optimize, especially for constant inputs, and that is overall less desirable.

[tannergooding]

I opened #94226 to fix up some of the cases like operator + so that the relevant inlining can occur.

[tcortega]

I opened #94226 to fix up some of the cases like operator + so that the relevant inlining can occur.

Great, I already have the solution you wanted implemented, now I just have to write a few unit tests for it, although I'm not really certain what to test in specific. I guess I should test overflows and correct TimeSpans?

[tarekgh]

public

All APIs need to have the Xml doc elements.

[tcortega]

Haha I originally wrote it with the XML Docs but noticed that the older methods do not have any documentation and removed them. No problem.

[danmoseley]

Yes we are slowly adding missing XML docs through these libraries. Historically docs were not generated from sources (unlike most .NET libraries out there - perhaps because many were written so early that there was not a useful docs tool chain). As we copy the official docs back onto the API (aided by tools) we can switch one library after another to being the "source of truth" for its API docs. Eventually they all will be. There is an ongoing project for this in case you are interested in contributing to it at some point.

[tarekgh]

I went ahead and took the liberty to add an public static System.TimeSpan FromMilliseconds(long milliseconds); that was not detailed in the proposal, but felt like it does not make sense to not add it.

It doesn't make sense to have FromMilliseconds(long milliseconds);. The proposal already has public static TimeSpan FromMilliseconds(long milliseconds, long microseconds = 0);

[tcortega]

I went ahead and took the liberty to add an public static System.TimeSpan FromMilliseconds(long milliseconds); that was not detailed in the proposal, but felt like it does not make sense to not add it.

It doesn't make sense to have FromMilliseconds(long milliseconds);. The proposal already has public static TimeSpan FromMilliseconds(long milliseconds, long microseconds = 0);

The main benefit appears to be the conservation of CPU cycles. Otherwise, why have both TimeSpan FromDays(int days) and an overloaded TimeSpan FromDays(int days, int hours = 0, ...) if the latter could suffice? It seems the oversight here is the lack of a TimeSpan FromMilliseconds(long milliseconds) method.

[tarekgh]

The main benefit appears to be the conservation of CPU cycles. Otherwise, why have both TimeSpan FromDays(int days) and an overloaded TimeSpan FromDays(int days, int hours = 0, ...) if the latter could suffice? It seems the oversight here is the lack of a TimeSpan FromMilliseconds(long milliseconds) method.

Agree, in the approved overloads, the second parameter shouldn't be defaulted on. @bartonjs @tannergooding do you have thoughts about that?

[tcortega]

The main benefit appears to be the conservation of CPU cycles. Otherwise, why have both TimeSpan FromDays(int days) and an overloaded TimeSpan FromDays(int days, int hours = 0, ...) if the latter could suffice? It seems the oversight here is the lack of a TimeSpan FromMilliseconds(long milliseconds) method.

Agree, in the approved overloads, the second parameter shouldn't be defaulted on. @bartonjs @tannergooding do you have thoughts about that?

Btw, if the purpose of these methods is to save CPU cycles, wouldn't this imply that we need to implement overflow checks repeatedly? Without these checks, we'd still incur the CPU cost for computations like 0 * N + N..., regardless of their simplicity.

[tarekgh]

Btw, if the purpose of these methods is to save CPU cycles

I don't think this is the main goal. The main goal is to provide API produce accurate results no rounding or truncation as the overloads that take double. Still be good to optimize as much as we can as a secondary goal.

[tcortega]

Btw, if the purpose of these methods is to save CPU cycles

I don't think this is the main goal. The main goal is to provide API produce accurate results no rounding or truncation as the overloads that take double. Still be good to optimize as much as we can as a secondary goal.

I understand that delivering precise results is the primary objective. However, my point is that the rationale for having distinct methods like TimeSpan FromDays(int days) and TimeSpan FromDays(int days, int hours = 0, long minutes = 0, long seconds = 0, long milliseconds = 0, long microseconds = 0) would be to enhance their performance; without such an improvement, their separate existence wouldn't be justifiable.

[tarekgh]

without such an improvement, their separate existence wouldn't be justifiable.

To be honest, I have a different perspective. The "From" methods we offer allow you to create a time span without needing to provide all the parameters that constructors require. This is why we are utilizing the default parameters. I am not saying we should not care about performance. I am trying to say we should first ensure the precision and user experience of the APIs usage first and then try to make the APIs performant without scarifying the main goal.

[tcortega]

without such an improvement, their separate existence wouldn't be justifiable.

To be honest, I have a different perspective. The "From" methods we offer allow you to create a time span without needing to provide all the parameters that constructors require. This is why we are utilizing the default parameters. I am not saying we should not care about performance. I am trying to say we should first ensure the precision and user experience of the APIs usage first and then try to make the APIs performant without scarifying the main goal.

But from the user's perspective, what would be the difference if we removed the TimeSpan FromDays(int days)?

[bartonjs]

Otherwise, why have both TimeSpan FromDays(int days) and an overloaded TimeSpan FromDays(int days, int hours = 0, ...) if the latter could suffice?

Usability studies have shown that junior/novice developers get confused with the IntelliSense presentation of methods with many default parameters. That resulted in this guideline in Framework Design Guidelines, 3rd edition (specifically in 5.1.1 (Member Overloading), p143): DO provide a simple overload, with no default parameters, for any method with two or more defaulted parameters.. FromDays had more than 1. FromMilliseconds only had 1.

[bartonjs]

There's also room for improved performance (from various reasons) from the shorter overloads, but the reason they got added in API Review was because of the usability guideline, not a performance goal.

[tarekgh]

@bartonjs

Usability studies have shown that junior/novice developers get confused with the IntelliSense presentation of methods with many default parameters.

Should we add public static TimeSpan FromMilliseconds(long milliseconds); to the proposal then?

Also, in the approved APIs, should all overloads that taking optional parameters has the second argument be not defaulted? like

    public static TimeSpan FromDays(int days);
    public static TimeSpan FromDays(int days, int hours = 0, long minutes = 0, long seconds = 0, long milliseconds = 0, long  microseconds = 0);

the hours parameter shouldn't be defaulted. or do we have this if someone uses named arguments?

[bartonjs]

Should we add public static TimeSpan FromMilliseconds(long milliseconds); to the #93890 (comment) then?

No, because the current method for it does not have two or more defaulted parameters. It has one mandatory parameter and one optional/defaulted parameter.

Also, in the approved APIs, should all overloads that taking optional parameters has the second argument be not defaulted? like ... the hours parameter shouldn't be defaulted. or do we have this if someone uses named arguments?

The way that it got approved is correct. The number of hours in FromDays should, indeed, be defaulted so that someone can call FromDays(192, minutes: 3), if they like.

[tarekgh]

@tcortega let's continue with the original approved proposal. Could you please update the code according to the discussions here? Thanks!

[ghost]

This pull request has been automatically marked no-recent-activity because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will remove no-recent-activity.

[tarekgh]

@tcortega are you willing to finish this PR. @stefannikolei is interested in helping too. Thanks!

[tcortega]

@tcortega are you willing to finish this PR. @stefannikolei is interested in helping too. Thanks!

Hey, I can't do it right now, so @stefannikolei, feel free to go ahead with it. Thanks!

[tarekgh]

@stefannikolei please create a new PR based on the discussions in this PR. Thanks @tcortega for letting us know.