Performance improvements and more benchmarks#347
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #347 +/- ##
==========================================
- Coverage 97.94% 97.94% -0.01%
==========================================
Files 40 40
Lines 52924 52949 +25
Branches 1115 1124 +9
==========================================
+ Hits 51839 51862 +23
- Misses 851 852 +1
- Partials 234 235 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📊 Benchmark Results
PR branch
|
| Method | Job | Runtime | PhoneNumberCount | Mean | Error | StdDev | Gen0 | Gen1 | Allocated |
|---|---|---|---|---|---|---|---|---|---|
| ParseValidateAndFormatPhoneNumbers | .NET 8.0 | .NET 8.0 | 1000 | 2.719 ms | 0.0165 ms | 0.0154 ms | 35.1563 | - | 582.48 KB |
| ParseValidateAndFormatPhoneNumbers | .NET 9.0 | .NET 9.0 | 1000 | 2.515 ms | 0.0066 ms | 0.0052 ms | 35.1563 | - | 582.48 KB |
| ParseValidateAndFormatPhoneNumbers | .NET Framework 4.8 | .NET Framework 4.8 | 1000 | 7.511 ms | 0.0268 ms | 0.0251 ms | 226.5625 | 23.4375 | 1397.64 KB |
| ParseValidateAndFormatPhoneNumbers | .NET 8.0 | .NET 8.0 | 10000 | 26.832 ms | 0.0589 ms | 0.0522 ms | 343.7500 | - | 5818.97 KB |
| ParseValidateAndFormatPhoneNumbers | .NET 9.0 | .NET 9.0 | 10000 | 25.445 ms | 0.0696 ms | 0.0617 ms | 343.7500 | - | 5818.97 KB |
| ParseValidateAndFormatPhoneNumbers | .NET Framework 4.8 | .NET Framework 4.8 | 10000 | 75.861 ms | 1.0163 ms | 0.9010 ms | 2142.8571 | 142.8571 | 13952.15 KB |
| ParseValidateAndFormatPhoneNumbers | .NET 8.0 | .NET 8.0 | 100000 | 274.642 ms | 2.1780 ms | 1.9308 ms | 3500.0000 | - | 58205.32 KB |
| ParseValidateAndFormatPhoneNumbers | .NET 9.0 | .NET 9.0 | 100000 | 256.281 ms | 0.6291 ms | 0.5254 ms | 3500.0000 | - | 58205.32 KB |
| ParseValidateAndFormatPhoneNumbers | .NET Framework 4.8 | .NET Framework 4.8 | 100000 | 756.903 ms | 7.1415 ms | 5.9635 ms | 22000.0000 | 2000.0000 | 139529.64 KB |
There was a problem hiding this comment.
Pull request overview
This PR focuses on reducing runtime allocations in hot paths, compressing embedded metadata/mapping resources, and expanding the BenchmarkDotNet performance suite for the C# port of libphonenumber.
Changes:
- Compress generated binary metadata/geocoding/timezone resources with gzip and update runtime loaders/readers to transparently decompress.
- Reduce allocations in frequently executed code paths (matcher grouping validation, RFC3966 group splitting, AsYouTypeFormatter substring avoidance, etc.).
- Add multiple new BenchmarkDotNet benchmarks and document how to run them; introduce Codecov coverage gates.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| csharp/PhoneNumbers/Util.cs | Exposes internals to the performance test assembly. |
| csharp/PhoneNumbers/PrefixFileReader.cs | Decompresses gzipped prefix-map resources before deserialization. |
| csharp/PhoneNumbers/PhoneNumberUtil.cs | Uses generic Enum.GetValues where supported to reduce overhead. |
| csharp/PhoneNumbers/PhoneNumberToTimeZonesMapper.cs | Decompresses gzipped timezone map resource before deserialization. |
| csharp/PhoneNumbers/PhoneNumbers.csproj | Enables nullable on modern TFMs and opts into AOT/trim analysis (see review comments). |
| csharp/PhoneNumbers/PhoneNumberMatcher.cs | Reduces allocations in grouping validation and avoids intermediate substring+split. |
| csharp/PhoneNumbers/MetadataLoader.cs | Changes embedded metadata loading to return decompressed streams (gzip). |
| csharp/PhoneNumbers/AsYouTypeFormatter.cs | Avoids intermediate substring allocations when extracting national number. |
| csharp/PhoneNumbers.PerformanceTest/README.md | Documents additional available benchmarks. |
| csharp/PhoneNumbers.PerformanceTest/Benchmarks/AsYouTypeFormatterBenchmark.cs | New benchmark measuring per-keystroke formatting cost. |
| csharp/PhoneNumbers.PerformanceTest/Benchmarks/PhoneNumberMatcherBenchmark.cs | New benchmark for FindNumbers under different leniencies. |
| csharp/PhoneNumbers.PerformanceTest/Benchmarks/ParsingHelpersBenchmark.cs | New benchmark isolating ExtractPossibleNumber paths. |
| csharp/PhoneNumbers.PerformanceTest/Benchmarks/ColdStartBenchmark.cs | New cold-start benchmarks for first-use and lazy-load costs. |
| csharp/PhoneNumbers.MetadataBuilder/Program.cs | Writes gzip-compressed outputs for metadata/geocoding/timezones generation. |
| codecov.yml | Adds Codecov project/patch coverage thresholds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+30
to
+35
| <!-- | ||
| Opt in to trim/AOT analysis on modern .NET TFMs. IsAotCompatible turns on the trim, single-file, | ||
| and AOT analyzers, and stamps the assembly with the metadata that lets consumers PublishAot | ||
| without warnings about untrimmable references. netstandard2.0 consumers are unaffected. | ||
| --> | ||
| <IsAotCompatible>true</IsAotCompatible> |
Collaborator
There was a problem hiding this comment.
i think this might be legit to address
Owner
Author
There was a problem hiding this comment.
-
RegexOptions.Compiled is not annotated [RequiresDynamicCode]. The .NET regex engine detects AOT mode at runtime and silently falls back to the interpreted engine for any new Regex(pattern, RegexOptions.Compiled) call. Just a perf cost on whichever code paths build regexes dynamically. (See dotnet/runtime for Regex.Compile's AOT handling; the IL-emit path is guarded by an IsDynamicCodeSupported check.) That's why IsAotCompatible=true + TreatWarningsAsErrors=true builds clean.
-
All static patterns already use [GeneratedRegex] on net7+. Source-generated regexes are fully ahead-of-time-compiled and indistinguishable from RegexOptions.Compiled performance under AOT. The runtime new Regex(pattern, Compiled) paths in the codebase are the netstandard2.0/net48 fallbacks (those TFMs don't set IsAotCompatible) plus the genuinely dynamic patterns in PhoneRegex / RegexCache whose patterns come from per-region metadata strings — these can't be source-generated regardless.
Collaborator
There was a problem hiding this comment.
i see, all good in that case
Comment on lines
126
to
+133
| public Stream? LoadMetadata(string fileName) | ||
| => assembly.GetManifestResourceStream(resourcePrefix + fileName); | ||
| { | ||
| // The build pipeline gzips every bin before embedding it (see GZipStream wrapping in | ||
| // PhoneNumbers.MetadataBuilder). Decompress on the way out so callers see the plain | ||
| // bin format they already expect. | ||
| var raw = assembly.GetManifestResourceStream(resourcePrefix + fileName); | ||
| return raw == null ? null : new GZipStream(raw, CompressionMode.Decompress); | ||
| } |
Collaborator
There was a problem hiding this comment.
i think this is fine since we use nuget packages
Owner
Author
There was a problem hiding this comment.
It's technically breaking in that what public Stream? LoadMetadata(string fileName) expects is changing but this was only added a month ago and I think it's safe to say no one is consuming it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes