Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do some cleanup on TimeSpan to ensure better inlining and clearer semantics #94226

Merged
merged 4 commits into from
Nov 1, 2023

Conversation

tannergooding
Copy link
Member

As per the title, this does some cleanup of TimeSpan to more consistently use throw helpers, to more consistently use named constants for important values, and to ensure that the implementation lives in the operators so less inlining is needed in the common case.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 31, 2023
@ghost ghost assigned tannergooding Oct 31, 2023
@tannergooding
Copy link
Member Author

Some of this will allow constants to be folded for #94143 if we went with the following implementation:

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);
}

@tannergooding tannergooding added area-System.DateTime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Oct 31, 2023
@ghost
Copy link

ghost commented Oct 31, 2023

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

Issue Details

As per the title, this does some cleanup of TimeSpan to more consistently use throw helpers, to more consistently use named constants for important values, and to ensure that the implementation lives in the operators so less inlining is needed in the common case.

Author: tannergooding
Assignees: tannergooding
Labels:

area-System.DateTime

Milestone: -

@tarekgh tarekgh added this to the 9.0.0 milestone Oct 31, 2023
Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. thanks for cleaning this up!


if ((t1Sign == (t2._ticks >> 63)) && (t1Sign != (result >> 63)))
{
// Overflow if signs of operands was identical and result's sign was opposite.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just curious, you could have used >>> instead of >> here, right? then it would only be the low order bit holding the sign. does it make any difference to perf?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed and unsigned shift have the same cost in x64 and Arm64 hardware, so no real difference

@tannergooding tannergooding deleted the timespan-simple branch November 1, 2023 04:28
@ghost ghost locked as resolved and limited conversation to collaborators Dec 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants