-
Notifications
You must be signed in to change notification settings - Fork 285
Optimize CoverageByXPathLevels data model and trie queries #8333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+491
−216
Merged
Changes from all commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
a94ab0d
test_export_language_identifier_display_names
sffc df937bf
🤖 Optimize CoverageByXPathLevels data model and trie queries for disp…
sffc 70c1aa4
make the test slightly faster
sffc 281345f
🤖 Fix clippy warnings in displaynames coverage module
sffc 4eba8a7
reduce diff
sffc f992f45
🤖 Fix redundant closure clippy warning in e2e test
sffc 26b0119
🤖 Move e2e benchmark test to standalone benches/displaynames.rs binary
sffc a66c6a3
🤖 Simplify parse_cldr_xpath string splitting using split_once
sffc 2e62ba2
🤖 Refactor CoverageByXPathLevels field selection to use CoverageCateg…
sffc 923036a
🤖 Implement custom Serde deserializer for CoverageByXPathLevels to el…
sffc cf7f0c5
🤖 Remove is_language parameter and use writeable::adapters::Replace u…
sffc a15a6c9
🤖 Update macro and function doc comments and restore CoverageByXPathL…
sffc 8ae0339
🤖 Handle Unknown variants in Alt::as_str and Menu::as_str by returnin…
sffc 3b5fd98
🤖 Normalize subtags using icu_locale_core during deserialization and …
sffc 3146bfe
🤖 Add module documentation to displaynames benchmark
sffc cd13bf9
🤖 Clarify CoverageCategoryLevels doc comment
sffc a30753e
🤖 Use named struct fields in XPathArraySeed and XPathArrayVisitor
sffc efe4501
🤖 Import Cow at top of coverage_experimental module
sffc 80b422a
🤖 Add docstrings to Alt::as_str and Menu::as_str
sffc a2edf65
🤖 Add comment explaining non-ASCII XPath exclusion in visit_seq
sffc 2e747fe
🤖 Hoist root_levels lookup outside per-locale loop in displaynames it…
sffc 6e649fc
🤖 Import subtag types at file top in coverage_experimental
sffc File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| // This file is part of ICU4X. For terms of use, please see the file | ||
|
sffc marked this conversation as resolved.
|
||
| // called LICENSE at the top level of the ICU4X source tree | ||
| // (online at: https://github.com/unicode-org/icu4x/blob/main/LICENSE ). | ||
|
|
||
| //! Standalone benchmark measuring performance of exporting display names data | ||
| //! across modern coverage locales. | ||
| //! | ||
| //! Run using: | ||
| //! ```text | ||
| //! cargo bench -p icu_provider_source --bench displaynames | ||
| //! ``` | ||
|
|
||
| use icu_provider_export::blob_exporter::BlobExporter; | ||
| use icu_provider_export::prelude::*; | ||
| use icu_provider_source::SourceDataProvider; | ||
|
|
||
| fn main() { | ||
| let t0 = std::time::Instant::now(); | ||
| let provider = SourceDataProvider::new(); | ||
| let mut blob_bytes = Vec::new(); | ||
| let exporter = BlobExporter::new_with_sink(Box::new(&mut blob_bytes)); | ||
|
|
||
| let modern_locales = provider | ||
| .locales_for_coverage_levels([icu_provider_source::CoverageLevel::Modern]) | ||
| .unwrap(); | ||
|
|
||
| ExportDriver::new( | ||
| modern_locales | ||
| .into_iter() | ||
| .map(DataLocaleFamily::without_descendants), | ||
| DeduplicationStrategy::None.into(), | ||
| LocaleFallbacker::try_new_unstable(&provider).unwrap(), | ||
| ) | ||
| .with_markers( | ||
| icu::experimental::provider::MARKERS | ||
| .iter() | ||
| .copied() | ||
| .filter(|info| info.id.name().starts_with("LocaleNames")), | ||
| ) | ||
| .export(&provider, exporter) | ||
| .unwrap(); | ||
|
|
||
| let elapsed = t0.elapsed(); | ||
| println!( | ||
| "displaynames bench: {:.3?} s, blob size: {} bytes", | ||
| elapsed.as_secs_f64(), | ||
| blob_bytes.len() | ||
| ); | ||
| } | ||
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it to a benchmark
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also don't think it's useful as a benchmark
in any case, it should use our standard benchmark scaffolding
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was intentional in making it a plain
mainfn. 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.