Add offline geocoder benchmarks, and fix the crash they exposed - #372
Merged
Conversation
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (66.66%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #372 +/- ##
=======================================
Coverage 98.00% 98.01%
=======================================
Files 39 39
Lines 52798 52799 +1
Branches 1101 1103 +2
=======================================
+ Hits 51747 51751 +4
+ Misses 797 795 -2
+ Partials 254 253 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
📊 Benchmark Results
PR branch
PR base
|
twcclegg
changed the title
This was referenced Aug 14, 2026
Closed
Closed
This was referenced Aug 23, 2026
Closed
Closed
This was referenced Aug 31, 2026
Open
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
1 participant
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.
PhoneNumberOfflineGeocoderhad no benchmark coverage, so geocoding regressions are invisible. Thefirst run of the new benchmarks immediately found a crash bug and a hole in the perf gate, so this is
now four commits.
test: add benchmarks for the offline geocoderGetDescriptionForNumber— end-to-end over the standard benchmark data set, warmed so itmeasures steady-state lookups rather than lazy prefix-map loading.
GetDisplayCountry— one locale per resolvable region, isolating the country-name table behindthe fallback used when a number has no finer-grained area description.
The second exists because that path has a concrete, non-breaking inefficiency worth measuring:
Locale.GetDisplayCountrywalks a four-step language fallback and re-probesLocaleData.Data[country]on every step, where one probe would do.fix: stop the geocoder throwing for regions with no localised nameGetCountryNameindexedLocaleData.Data[country]unguarded.LocaleDatais generated fromjava.util.Locale.getISOCountries(), but AC (calling code 247) and XK are phone-metadataregions that aren't ISO countries, so they have no entry.
GetRegionDisplayNameonly guardsnull,ZZand001, so:A public API throwing on valid input, and a porting divergence — Java's
getDisplayCountryfallsback to the region code and never throws. Now returns null so the caller's language fallback runs and
the result is an empty description, which is what
GetDescriptionForNumberalready documents for aregion it can't name. The dedup-pointer lookup (
names[name.Substring(1)]) got the same treatment.The benchmark found this by accident: its setup called
GetDisplayCountryto filter unresolvableregions and died on
AC.ci: fail the perf job when a benchmark produces no resultThe bug above did not fail CI. BenchmarkDotNet exits 0 when a benchmark throws — it prints
NAforthat row and carries on — so
run_performance_testspassed in 10m11s while measuring nothing:The job now scans both result sets for rows with no measurement and fails, after the artifact upload
so partial results stay available.
Landing separately from any geocoder optimisation so the base commit has a counterpart to compare
against — a benchmark added in the same PR as the change it measures reports nothing, because the
comparison runs the base commit's suite against the branch's.