Optimize CoverageByXPathLevels data model and trie queries by sffc · Pull Request #8333 · unicode-org/icu4x

sffc · 2026-08-07T08:01:27Z

Redesigns CoverageByXPathLevels into 4 category-specific ZeroTrie fields (language, territory, script, variant), extracting subtag attribute keys and integer coverage level discriminators.

Stores only the short XPath suffixes in the trie and hoists per-locale coverage level lookups outside subtag loops.

Measured performance impact on test_export_language_identifier_display_names:

Full test runtime reduced from 19.15 s to 14.21 s (25.8% runtime reduction).
coverage_tier trie lookup time reduced from 9.60 s to 2.62 s (3.66x speedup).

🤖 This pull request was created by an AI agent working with @sffc.

Changelog

N/A

…lay names Redesign CoverageByXPathLevels into 4 category-specific ZeroTrie fields (language, territory, script, variant), extracting subtag attribute keys and integer coverage level discriminators. Hoist per-locale coverage level lookups outside subtag loops and eliminate string allocations during trie traversal. Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

sffc · 2026-08-07T08:17:09Z

At a94ab0d:

     Running tests/e2e.rs (/usr/local/google/home/sffc/projects/icu4x/target/debug/deps/e2e-b73e9b9cff5341b6)

running 1 test
test test_export_language_identifier_display_names ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 19.65s

At df937bf:

     Running tests/e2e.rs (/usr/local/google/home/sffc/projects/icu4x/target/debug/deps/e2e-b73e9b9cff5341b6)

running 1 test
test test_export_language_identifier_display_names ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 14.54s

That's a significant enough improvement that I think it's worth landing this.

I don't intend to spend more time than I already have on code that will most likely be mostly deleted.

Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

sffc · 2026-08-07T08:24:34Z

-            // CLDR coverage files contain some non-ASCII XPaths, principally emoji annotations
-            // (e.g., //ldml/annotations/annotation[@cp="😀"]).


this comment is obsolete since now we allowlist the XPaths we want instead of ingesting all of them

sffc · 2026-08-07T08:26:13Z

+                    let coverage_cache =
+                        crate::displaynames::coverage_experimental::coverage_cldr_cache();
+                    let locale_levels = coverage_cache.get_levels_for_locale(&locale, cldr)?;
+                    let root_levels = coverage_cache.get_root_levels()?;


Pulling these out of the inner loop is a low-cost way to reduce hits on read_and_parse

Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Copilot

Pull request overview

Redesigns the experimental CLDR coverage lookup used by display names providers to avoid XPath string construction/allocations and to speed up trie queries by indexing coverage tiers by display-name category + subtag (+ optional alt/menu). Adds an end-to-end export test exercising locale display name markers.

Changes:

Refactors CoverageByXPathLevels into four category-specific ZeroTrieSimpleAscii tries and updates coverage tier lookup APIs accordingly.
Updates all display name providers/macros to use DisplayNameCategory-based coverage lookups (and hoists per-locale coverage level fetches out of inner loops).
Adds an e2e export test and enables the blob_exporter dev-dependency feature, plus a networking-gated integration test target.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
provider/source/tests/e2e.rs	Adds an integration test exporting LocaleNames markers to a blob and asserting a minimum output size.
provider/source/src/displaynames/variant.rs	Switches coverage lookups from XPath prefix strings to `DisplayNameCategory::Variant`.
provider/source/src/displaynames/script.rs	Switches coverage lookups from XPath prefix strings to `DisplayNameCategory::Script` (all tiers/alt variants).
provider/source/src/displaynames/region.rs	Switches coverage lookups from XPath prefix strings to `DisplayNameCategory::Territory`.
provider/source/src/displaynames/language.rs	Switches coverage lookups from XPath prefix strings to `DisplayNameCategory::Language` (including menu providers).
provider/source/src/displaynames/mod.rs	Updates provider macros to use category-based coverage lookups and hoists per-locale coverage level retrieval in iterators.
provider/source/src/displaynames/coverage_experimental.rs	Implements the new category-specific trie model, cursor-based lookup, and new coverage APIs/tests.
provider/source/src/cldr_serde/displaynames/mod.rs	Adds `as_str()` helpers for `Alt`/`Menu` to support allocation-free cursor writes.
provider/source/Cargo.toml	Enables `icu_provider_export`’s `blob_exporter` feature and adds a `networking`-gated `e2e` test target.

Suppressed comments (2)

provider/source/src/displaynames/mod.rs:185

The macro signature now takes $category, but the doc comment doesn’t list/document it.

/// - `$resource`: The CLDR serde resource type.
/// - `$file`: The JSON file name in CLDR.
/// - `$field`: The field name in `LocaleDisplayNames` containing the data.
/// - `$tier`: The target coverage tier.
macro_rules! impl_displaynames_menu_v1 {
    ($marker:ident, $subtag_ty:ty, $resource:path, $file:literal, $field:ident, $category:expr, $tier:pat,) => {

provider/source/src/displaynames/mod.rs:339

The macro signature now takes $category, but the doc comment doesn’t document this parameter.

/// - `$resource`: The CLDR serde resource type.
/// - `$file`: The JSON file name in CLDR.
/// - `$field`: The field name in `LocaleDisplayNames` containing the data.
/// - `$alt_variant`: The alt variant (e.g., `None`, `Some(Alt::Short)`).
/// - `$tier`: The target coverage tier.
macro_rules! impl_displaynames_iter_v1 {
    ($marker:ident, $subtag_ty:ty, $resource:path, $file:literal, $field:ident, $alt_variant:expr, $category:expr, $tier:pat) => {

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -95,7 +95,7 @@ where
 /// - `$alt_variant`: The alt variant (e.g., `None`, `Some(Alt::Short)`).
 /// - `$tier`: The target coverage tier.
 macro_rules! impl_displaynames_v1 {
-    ($marker:ident, $subtag_ty:ty, $resource:path, $file:literal, $field:ident, $alt_variant:expr, $xpath_prefix:literal, $tier:pat,) => {
+    ($marker:ident, $subtag_ty:ty, $resource:path, $file:literal, $field:ident, $alt_variant:expr, $category:expr, $tier:pat,) => {


    cldr: &CldrCache,
    file_name: &str,
-    xpath_prefix: &str,
+    category: DisplayNameCategory,
    mut extract_keys: impl FnMut(&Resource) -> &HashMap<WithAlt<T>, String>,


sffc · 2026-08-10T07:16:49Z

+use icu_provider_source::SourceDataProvider;
+
+#[test]
+fn test_export_language_identifier_display_names() {


This test exists for performance benchmarking purposes. I could probably make it #[ignore]. I could maybe move it to a benches module.

robertbastian · 2026-08-10T15:52:27Z

I don't see the value of this test. It was probably useful while you were working on performance improvements, but I don't think it should be checked in.

I moved it to a benchmark

also don't think it's useful as a benchmark

in any case, it should use our standard benchmark scaffolding

I was intentional in making it a plain main fn. It is too slow for criterion, and I don't want to pull in other dependencies.

I think this is valuable as a benchmark because this covers a lot of datagen code paths, and improving the performance of this benchmark can improve the performance of datagen in general. Datagen in general is getting quite slow, so benchmarks seem warranted.

robertbastian · 2026-08-10T15:54:49Z

+    let quote_idx = rest.find('"')?;
+    let val = &rest[..quote_idx];
+    let remaining_attrs = &rest[quote_idx + 1..];
+    let extra_attrs = remaining_attrs.strip_prefix(']')?;


this is split_once("\"]")

robertbastian · 2026-08-10T16:00:08Z

-            Some(CoverageLevelForXPath::Modern)
-        } else {
-            None
+impl TryFrom<RawCoverageByXPathLevels> for CoverageByXPathLevels {


suggestion: write a custom serde impl instead of using TryFrom. that way you don't have to collect into intermediate Vecs, and I also think it will make this code more readable

ok, I think either structure is fine but I'll do the one you suggest

robertbastian · 2026-08-10T16:01:23Z

+impl CoverageByXPathLevels {
+    pub(super) fn level_for_category(
+        &self,
+        category: DisplayNameCategory,


instead of creating an enum to select the field on CoverageByXPathLevels, just let the caller select the field. you'll need a newtype around the trie for the remaining logic

ok, I think either structure is fine but I'll do the one you suggest

Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

…oryLevels newtype wrapper Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

…iminate intermediate Vec allocations Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

…nconditionally Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

…evels doc comment Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

sffc · 2026-08-10T18:13:46Z

I directed and monitored jetski to fix these issues. I'm conflicted because I want to land the 25% performance improvement on displaynames datagen, but I don't want to spend more time on code that is likely to be largely deleted once we get the new CLDR coverage levels.

…g Option<&'static str> Co-authored-by: 176961590+gemini-code-assist[bot]@users.noreply.github.com

…remove Writeable Replace Co-authored-by: 176961590+gemini-code-assist[bot]@users.noreply.github.com

Co-authored-by: 176961590+gemini-code-assist[bot]@users.noreply.github.com

…er macros Co-authored-by: 176961590+gemini-code-assist[bot]@users.noreply.github.com

Co-authored-by: 176961590+gemini-code-assist[bot]@users.noreply.github.com

robertbastian

.

sffc · 2026-08-12T20:27:46Z

.

The only open conversation, I think, is the one about whether or not to land the benchmark. I don't consider this a blocking issue: the benchmark file can be easily deleted later without breaking anyone, and it does not slow down our tests or have other negative impact on our processes.

(As stated above, I think it is useful to reproduce and measure this part of datagen, especially since we will be rewriting this code again come CLDR 48.)

So I think this PR is landable now.

@Manishearth can you take a look?

Manishearth

Approving based on:

Shane's assertion in chat that this is not intended to be code read deeply
Understanding the PR body
Quickly giving a once over to the code, which is rather complicated and not commented much
Understanding the actual code has been through multiple review passes already
Understanding that the open review issue on benchmarks will be resolved later
Knowing all this code will go away in the future (please ensure the followup CLDR version cleans up as much of this as possible)

sffc · 2026-08-12T21:57:28Z

I want to delete basically this entire file once CLDR 49 is available. #8362

robertbastian · 2026-08-13T08:56:19Z

Approving based on:

Shane's assertion in chat that this is not intended to be code read deeply

I didn't know we could do that!

sffc and others added 3 commits August 6, 2026 22:51

test_export_language_identifier_display_names

a94ab0d

make the test slightly faster

70c1aa4

sffc and others added 2 commits August 7, 2026 01:22

🤖 Fix clippy warnings in displaynames coverage module

281345f

Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

reduce diff

4eba8a7

sffc commented Aug 7, 2026

View reviewed changes

🤖 Fix redundant closure clippy warning in e2e test

f992f45

Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

sffc marked this pull request as ready for review August 7, 2026 08:37

sffc requested review from Manishearth and robertbastian as code owners August 7, 2026 08:37

sffc mentioned this pull request Aug 7, 2026

Introduce DataIdentifierCached wrapping DataIdentifierCow #8334

Open

sffc requested a review from Copilot August 7, 2026 17:31

Copilot started reviewing on behalf of sffc August 7, 2026 17:32 View session

Copilot AI reviewed Aug 7, 2026

View reviewed changes

sffc commented Aug 10, 2026

View reviewed changes

robertbastian reviewed Aug 10, 2026

View reviewed changes

sffc and others added 3 commits August 10, 2026 10:43

🤖 Move e2e benchmark test to standalone benches/displaynames.rs binary

26b0119

Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

🤖 Simplify parse_cldr_xpath string splitting using split_once

a66c6a3

Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

🤖 Refactor CoverageByXPathLevels field selection to use CoverageCateg…

2e62ba2

…oryLevels newtype wrapper Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

sffc commented Aug 10, 2026

View reviewed changes

Comment thread provider/source/src/displaynames/coverage_experimental.rs Outdated

Comment thread provider/source/src/displaynames/coverage_experimental.rs Outdated

sffc and others added 3 commits August 10, 2026 11:01

🤖 Implement custom Serde deserializer for CoverageByXPathLevels to el…

923036a

…iminate intermediate Vec allocations Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

🤖 Remove is_language parameter and use writeable::adapters::Replace u…

cf7f0c5

…nconditionally Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

🤖 Update macro and function doc comments and restore CoverageByXPathL…

a15a6c9

…evels doc comment Co-authored-by: Gemini <176961590+gemini-code-assist[bot]@users.noreply.github.com>

sffc requested a review from robertbastian August 10, 2026 18:09

sffc added 4 commits August 10, 2026 12:18

🤖 Handle Unknown variants in Alt::as_str and Menu::as_str by returnin…

8ae0339

…g Option<&'static str> Co-authored-by: 176961590+gemini-code-assist[bot]@users.noreply.github.com

🤖 Normalize subtags using icu_locale_core during deserialization and …

3b5fd98

…remove Writeable Replace Co-authored-by: 176961590+gemini-code-assist[bot]@users.noreply.github.com

🤖 Add module documentation to displaynames benchmark

3146bfe

Co-authored-by: 176961590+gemini-code-assist[bot]@users.noreply.github.com

🤖 Clarify CoverageCategoryLevels doc comment

cd13bf9

Co-authored-by: 176961590+gemini-code-assist[bot]@users.noreply.github.com

sffc-bot reviewed Aug 10, 2026

View reviewed changes

sffc added 6 commits August 10, 2026 12:31

🤖 Use named struct fields in XPathArraySeed and XPathArrayVisitor

a30753e

Co-authored-by: 176961590+gemini-code-assist[bot]@users.noreply.github.com

🤖 Import Cow at top of coverage_experimental module

efe4501

Co-authored-by: 176961590+gemini-code-assist[bot]@users.noreply.github.com

🤖 Add docstrings to Alt::as_str and Menu::as_str

80b422a

Co-authored-by: 176961590+gemini-code-assist[bot]@users.noreply.github.com

🤖 Add comment explaining non-ASCII XPath exclusion in visit_seq

a2edf65

Co-authored-by: 176961590+gemini-code-assist[bot]@users.noreply.github.com

🤖 Hoist root_levels lookup outside per-locale loop in displaynames it…

2e747fe

…er macros Co-authored-by: 176961590+gemini-code-assist[bot]@users.noreply.github.com

🤖 Import subtag types at file top in coverage_experimental

6e649fc

Co-authored-by: 176961590+gemini-code-assist[bot]@users.noreply.github.com

sffc added this to the 2.3 Release ⟨P1⟩ milestone Aug 12, 2026

sffc added the milestone-non-blocking These issues are in a milestone, but do not block the milestone (and can be removed if necessary) label Aug 12, 2026

robertbastian reviewed Aug 12, 2026

View reviewed changes

Manishearth approved these changes Aug 12, 2026

View reviewed changes

sffc merged commit 0a69936 into unicode-org:main Aug 12, 2026
35 checks passed

sffc deleted the dname-export-perf branch August 12, 2026 21:57

		// CLDR coverage files contain some non-ASCII XPaths, principally emoji annotations
		// (e.g., //ldml/annotations/annotation[@cp="😀"]).

Uh oh!

Conversation

sffc commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog

Uh oh!

sffc commented Aug 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sffc Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sffc Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sffc commented Aug 10, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robertbastian left a comment

Choose a reason for hiding this comment

Uh oh!

sffc commented Aug 12, 2026

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

sffc commented Aug 12, 2026

Uh oh!

Uh oh!

robertbastian commented Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sffc commented Aug 7, 2026 •

edited

Loading

sffc Aug 12, 2026 •

edited

Loading

sffc Aug 11, 2026 •

edited

Loading