Improve ordinal and clock notation hot paths#1732
Conversation
There was a problem hiding this comment.
Pull request overview
Optimizes Humanizer’s ordinalization and phrase-based clock-notation conversion hot paths to reduce allocations and avoid unnecessary culture/word-resolution work.
Changes:
- Refactors
int.Ordinalize(..., CultureInfo)to avoid loadingCultureInfo.NumberFormaton the positive-number path and to better preserve custom negative-sign behavior for negative values. - Updates phrase clock-notation template expansion to lazily resolve placeholder values only when used by the selected template.
- Adds a regression test covering custom
NegativeSignhandling for negative ordinalization.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/Humanizer.Tests/OrdinalizeTests.cs | Adds a test ensuring custom culture negative-sign is preserved for negative ordinals. |
| src/Humanizer/OrdinalizeExtensions.cs | Introduces a new ordinal number-string formatting path intended to reduce culture formatting overhead. |
| src/Humanizer/Localisation/TimeToClockNotation/PhraseClockNotationConverter.cs | Lazily resolves template placeholders to avoid computing unused hour/minute/article/day-period strings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 51 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 59 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
int.Ordinalize(culture)positive-number regression found in the benchmark artifactsBenchmark Evidence
Local net10 short BenchmarkDotNet runs on Apple M2 Max after
b959f088:MetricNumeralBenchmarks: small59.05 ns / 56 B, kilo117.68 ns / 112 B, mega113.29 ns / 112 B, boundary149.74 ns / 128 B, giga158.14 ns / 152 B, formatted fallback205.21 ns / 128 B, milli125.57 ns / 136 BFormatterBenchmarks: Russian single-part TimeSpan95.84 ns / 64 B, multi-part control423.60 ns / 600 B, zero161.50 ns / 80 B, words131.71 ns / 224 B, Romanian Date140.24 ns / 136 B, Arabic DataUnit39.56 ns / 80 BShort local review-fix spot checks after
58084558:OrdinalBenchmarks: English ordinalize29.93 ns / 64 B; custom-culture paths unchanged in shape for Dutch/Turkish/Greek/Finnish control casesTransformersBenchmarks: title case 10 chars201.24 ns / 696 B, 100 chars2.424 us / 7264 B, 1000 chars24.483 us / 73200 BShort local ordinal regression check after
3221a0ad:OrdinalBenchmarks: English27.63 ns / 64 B, Dutch44.51 ns / 176 B, Turkish45.18 ns / 176 B, Greek46.37 ns / 176 B, Finnish44.53 ns / 176 B58084558showed Dutch/Turkish/Greek around110-111 us; the cache change removes that regression locally.GitHub benchmark workflow:
b959f088completed successfully: https://github.com/Humanizr/Humanizer/actions/runs/2461698567658084558completed successfully, but its comparison artifact exposed the ResultsComparer restore/reporting bug: https://github.com/Humanizr/Humanizer/actions/runs/246177400313221a0adis running from the push and PR events:Tests
dotnet build src/Benchmarks/Benchmarks.csproj -c Release -f net10.0dotnet run --project src/Benchmarks/Benchmarks.csproj -c Release -f net10.0 -- --filter '*MetricNumeralBenchmarks*' --job short --warmupCount 1 --iterationCount 3dotnet run --project src/Benchmarks/Benchmarks.csproj -c Release -f net10.0 -- --filter '*FormatterBenchmarks*' --job short --warmupCount 1 --iterationCount 3dotnet run --project src/Benchmarks/Benchmarks.csproj -c Release -f net10.0 -- --filter '*OrdinalBenchmarks*' --job short --warmupCount 1 --iterationCount 3dotnet run --project src/Benchmarks/Benchmarks.csproj -c Release -f net10.0 -- --filter '*TransformersBenchmarks*' --job short --warmupCount 1 --iterationCount 3dotnet test --project tests/Humanizer.Tests/Humanizer.Tests.csproj --framework net10.0 --no-restore -- --filter-class '*OrdinalizeTests' --filter-class '*TransformersTests' --filter-class '*CoverageGapTests'dotnet test --project tests/Humanizer.Tests/Humanizer.Tests.csproj --framework net10.0 --no-restore -- --filter-class '*OrdinalizeTests' --filter-class '*DutchGenderedOrdinalTests'dotnet test --project tests/Humanizer.Tests/Humanizer.Tests.csproj --framework net11.0 --no-restore -- --filter-class '*OrdinalizeTests' --filter-class '*DutchGenderedOrdinalTests'dotnet run --project src/Benchmarks/Benchmarks.csproj -c Release -f net10.0 -- --filter '*OrdinalBenchmarks*' --job short --warmupCount 1 --iterationCount 1dotnet format Humanizer.slnx --verify-no-changes --verbosity diagnostic --no-restoreruby -e 'require "yaml"; YAML.load_file(".github/workflows/benchmarks-baseline-vs-current.yml"); puts "yaml ok"'dotnet pack src/Humanizer/Humanizer.csproj -c Release -o artifacts/packages/ordinal-benchmark-fixdotnet test --project tests/Humanizer.Tests/Humanizer.Tests.csproj --framework net10.0 --no-restoredotnet test --project tests/Humanizer.Tests/Humanizer.Tests.csproj --framework net11.0 --no-restoregit diff --checkNote:
dotnet test --project tests/Humanizer.Tests/Humanizer.Tests.csproj --framework net8.0 --no-restorecould not run locally because this machine does not have the .NET 8 runtime installed; CI should cover it.