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

Trim some overheads for "r"/"o" DateTime parsing #82925

Merged
merged 1 commit into from
Mar 4, 2023

Conversation

stephentoub
Copy link
Member

We were doing some unnecessary work on these fast paths:

  • We were calling ConfigureFormatR/O even though the work they were doing was never used. Removed the calls and then inlined the code into the remaining call sites.
  • Removed unnecessary arguments.
  • Removed a virtual dispatch calling to GregorianCalendar when we could instead just go straight to the helper on DateTime.
  • Reduced number of cases in switch statement.
  • I also noticed and cleaned up an unnecessary delegate involved in Hebrew number parsing.
private string _r = new DateTime(1955, 11, 5, 6, 0, 0, DateTimeKind.Utc).ToString("r", CultureInfo.InvariantCulture);
private string _o = new DateTime(1955, 11, 5, 6, 0, 0, DateTimeKind.Utc).ToString("o", CultureInfo.InvariantCulture);

[Benchmark] public DateTimeOffset ParseR() => DateTimeOffset.ParseExact(_r, "r", CultureInfo.InvariantCulture);
[Benchmark] public DateTimeOffset ParseO() => DateTimeOffset.ParseExact(_o, "o", CultureInfo.InvariantCulture);
Method Toolchain Mean Error StdDev Ratio
ParseR \main\corerun.exe 42.55 ns 0.552 ns 0.636 ns 1.00
ParseR \pr\corerun.exe 38.56 ns 0.556 ns 0.520 ns 0.91
ParseO \main\corerun.exe 49.60 ns 0.501 ns 0.468 ns 1.00
ParseO \pr\corerun.exe 43.75 ns 0.402 ns 0.376 ns 0.88

We were doing some unnecessary work on these fast paths.

I also noticed and cleaned up an unnecessary delegate involved in Hebrew number parsing.
@ghost
Copy link

ghost commented Mar 3, 2023

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

Issue Details

We were doing some unnecessary work on these fast paths:

  • We were calling ConfigureFormatR/O even though the work they were doing was never used. Removed the calls and then inlined the code into the remaining call sites.
  • Removed unnecessary arguments.
  • Removed a virtual dispatch calling to GregorianCalendar when we could instead just go straight to the helper on DateTime.
  • Reduced number of cases in switch statement.
  • I also noticed and cleaned up an unnecessary delegate involved in Hebrew number parsing.
private string _r = new DateTime(1955, 11, 5, 6, 0, 0, DateTimeKind.Utc).ToString("r", CultureInfo.InvariantCulture);
private string _o = new DateTime(1955, 11, 5, 6, 0, 0, DateTimeKind.Utc).ToString("o", CultureInfo.InvariantCulture);

[Benchmark] public DateTimeOffset ParseR() => DateTimeOffset.ParseExact(_r, "r", CultureInfo.InvariantCulture);
[Benchmark] public DateTimeOffset ParseO() => DateTimeOffset.ParseExact(_o, "o", CultureInfo.InvariantCulture);
Method Toolchain Mean Error StdDev Ratio
ParseR \main\corerun.exe 42.55 ns 0.552 ns 0.636 ns 1.00
ParseR \pr\corerun.exe 38.56 ns 0.556 ns 0.520 ns 0.91
ParseO \main\corerun.exe 49.60 ns 0.501 ns 0.468 ns 1.00
ParseO \pr\corerun.exe 43.75 ns 0.402 ns 0.376 ns 0.88
Author: stephentoub
Assignees: stephentoub
Labels:

area-System.Globalization

Milestone: -

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

@stephentoub stephentoub merged commit f6d564e into dotnet:main Mar 4, 2023
@stephentoub
Copy link
Member Author

Thanks

@stephentoub stephentoub deleted the formatr branch March 4, 2023 01:06
@EgorBo
Copy link
Member

EgorBo commented Mar 9, 2023

Improvements on ubuntu-arm64:

@ghost ghost locked as resolved and limited conversation to collaborators Apr 8, 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