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

Simplify TimeSpanFormat #102145

Closed
wants to merge 2 commits into from
Closed

Simplify TimeSpanFormat #102145

wants to merge 2 commits into from

Conversation

lilinus
Copy link
Contributor

@lilinus lilinus commented May 13, 2024

I tried to consolidate some code in TimeSpanFormat (and minor change in DateTimeFormat).

No big performance improvements but I checked for regressions. Major gain for this PR is code is simpler, and a method in common source file is moved to CoreLib, which was the only place it was used.

Benchmarks (source)

| Method          | Job        | Toolchain | value               | fmt                                     | Mean      | Error    | StdDev   | Median    | Min       | Max       | Ratio | Gen0   | Allocated | Alloc Ratio |
|---------------- |----------- |-----------|-------------------- |---------------------------------------- |----------:|---------:|---------:|----------:|----------:|----------:|------:|-------:|----------:|------------:|
| DateTime_Format | Job-CQLODJ | main      | 01/01/0001 00:20:34 | yyyy\\-MM\\-dd\\.HH\\.mm\\.ss\\.FFFFFFF | 106.10 ns | 0.203 ns | 0.159 ns | 106.09 ns | 105.85 ns | 106.41 ns |  1.00 | 0.0306 |      64 B |        1.00 |
| DateTime_Format | Job-JQHOSH | pr        | 01/01/0001 00:20:34 | yyyy\\-MM\\-dd\\.HH\\.mm\\.ss\\.FFFFFFF |  99.43 ns | 0.342 ns | 0.303 ns |  99.46 ns |  98.96 ns |  99.90 ns |  0.94 | 0.0304 |      64 B |        1.00 |
| TimeSpan_Format | Job-CQLODJ | main      | 00:20:34.5678900    | d\\.hh\\.mm\\.ss\\.FFFFFFF              | 175.52 ns | 0.715 ns | 0.669 ns | 175.63 ns | 173.67 ns | 176.38 ns |  1.00 | 0.0265 |      56 B |        1.00 |
| TimeSpan_Format | Job-JQHOSH | pr        | 00:20:34.5678900    | d\\.hh\\.mm\\.ss\\.FFFFFFF              | 163.26 ns | 0.648 ns | 0.606 ns | 163.28 ns | 161.23 ns | 163.79 ns |  0.93 | 0.0267 |      56 B |        1.00 |
| TimeSpan_Format | Job-CQLODJ | main      | 00:20:34.5678900    | g                                       |  30.59 ns | 0.195 ns | 0.182 ns |  30.54 ns |  30.38 ns |  30.88 ns |  1.00 | 0.0228 |      48 B |        1.00 |
| TimeSpan_Format | Job-JQHOSH | pr        | 00:20:34.5678900    | g                                       |  29.94 ns | 0.105 ns | 0.098 ns |  29.91 ns |  29.79 ns |  30.09 ns |  0.98 | 0.0228 |      48 B |        1.00 |
| TimeSpan_Format | Job-CQLODJ | main      | 00:20:34.5678900    | G                                       |  31.69 ns | 0.072 ns | 0.067 ns |  31.71 ns |  31.54 ns |  31.80 ns |  1.00 | 0.0306 |      64 B |        1.00 |
| TimeSpan_Format | Job-JQHOSH | pr        | 00:20:34.5678900    | G                                       |  31.26 ns | 0.111 ns | 0.104 ns |  31.27 ns |  30.96 ns |  31.40 ns |  0.99 | 0.0305 |      64 B |        1.00 |

@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 May 13, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 13, 2024
// value = 1234 => retVal = 0, valueWithoutTrailingZeros = 1234
// value = 320900 => retVal = 2, valueWithoutTrailingZeros = 3209
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static int CountDecimalTrailingZeros(uint value, out uint valueWithoutTrailingZeros)
Copy link
Contributor Author

@lilinus lilinus May 13, 2024

Choose a reason for hiding this comment

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

Only usage prior to change was in TimeSpanFormat.TryFormatStandard

@@ -106,39 +107,35 @@ internal enum StandardFormat
g
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static void GetAbsoluteParts(TimeSpan value, out int days, out int hours, out int minutes, out int seconds, out int fraction)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to follow pattern in DateTime.GetTime

@huoyaoyuan huoyaoyuan 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 May 13, 2024
Copy link
Contributor

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

@tarekgh
Copy link
Member

tarekgh commented May 13, 2024

@lilinus Thank you for submitting the pull request. Typically, if a code change doesn't offer significant benefits, we aim to avoid merely cleaning up or refactoring it. While this change may pass the CI tests, there's always a chance of regression. We might consider accepting it in the future if we make other modifications to the code. Thanks once more.

@tarekgh tarekgh closed this May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.DateTime community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants