Improve (unittest) performance by a factor of ~10 (expected real-world improvements to be even higher) - #161
Merged
Conversation
PhoneRegex now uses Lazy<Regex> and the RegexCache now uses a ConcurrentDictionary internally. I have sacrificed the LRU cache, which is why the TestRegexInsertion currently fails. This means Regexes are currently cached indefinitely. I wonder in how many real-world scenario's caching a few hundred(?) regexes would be an actual problem. Unfortunately I have no numbers on the use of this class nor any idea about the environments this class is used in. But the added complexity of the LRU (and making it thread-safe) seems not worth it to me. Especially if this easy optimization alone results in a 10x improvement. My numbers for running all unittests went from ~18seconds to ~1.8 seconds on my 9 year old workstation. Will discuss further in #136
Closed
Contributor
Author
|
In my tests with real world data I've seen an performance improvement of about a factor 70 in, at least, parsing the phonenumbers. |
Owner
|
Assuming this is good to go, I'm happy to merge. Thanks! |
Contributor
Author
|
You may want to check my remarks here but, yes, this should be good to go 🚀 |
Contributor
Author
|
Hi @twcclegg Was there a problem merging? Or did you just not get around to it? If there's anything I can do to help, please let me know. |
Owner
|
I didn't get notifications of either of the last two messages for some reason :( , so that would be why. |
Contributor
Author
|
No worries and thank you! |
3 tasks
pull Bot
pushed a commit
to LoadsAForks/libphonenumber-csharp
that referenced
this pull request
Aug 31, 2026
Every existing benchmark here either repeats one region (cheap after the first call) or warms its whole diverse region set in GlobalSetup before the timed run starts (PhoneNumberWorkflowBenchmark's seed-data generation calls GetExampleNumberForType/IsValidNumber/Format against every supported region). Neither shape can see the cost of a region's genuinely first use in the process, which is exactly where two real regressions lived: the 2017 RegexOptions.Compiled + undersized RegexCache issue (~115x, fixed by PR twcclegg#161) and the 2026 PR twcclegg#325 Compiled-regex change (~100x cold-start cost per new region, not caught by its own benchmark because that benchmark's setup already pre-warms every region it measures). Add ColdStartBenchmark.FirstUseValidateAndFormat: fresh PhoneNumberUtil per iteration, a fixed 20-region list GlobalSetup never touches, one previously-unseen region's full Parse+IsValidNumber+Format per invocation. Locally: 19.6ms median vs FirstRegionLookup's 300us (metadata-load-only), cleanly separating the regex-compile cost from the metadata-load cost this class already measured. No wiring changes needed - run_performance_tests.yml already runs `--filter "*"` for both branch and base and diffs every case via lib/compare-benchmarks.js's Welch's-t-test + 20%-floor comparison, so the new case is covered automatically. Documented the two-regression history and the "why" for this specific benchmark shape in README.md so a future benchmark addition doesn't accidentally drop the property that makes this one work.
5 tasks
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
2 participants
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.
PhoneRegex now uses Lazy and the RegexCache now uses a ConcurrentDictionary internally.
I have sacrificed the LRU cache, which is why the TestRegexInsertion currently fails. This means Regexes are currently cached indefinitely. I wonder in how many real-world scenario's caching a few hundred(?) regexes would be an actual problem. Unfortunately I have no numbers on the use of this class nor any idea about the environments this class is used in. But the added complexity of the LRU (and making it thread-safe) seems not worth it to me. Especially if this easy optimization alone results in a 10x improvement.
My numbers for running all unittests went from ~18seconds to ~1.8 seconds on my 9 year old workstation. Will discuss further in #136