CodePointTrie support for normalizer and collator perf improvements#7768
CodePointTrie support for normalizer and collator perf improvements#7768hsivonen wants to merge 5 commits intounicode-org:mainfrom
Conversation
Manishearth
left a comment
Manishearth
left a comment
There was a problem hiding this comment.
Will take some time to properly review
| } | ||
|
|
||
| #[inline(always)] | ||
| unsafe fn get_bit_prefix_suffix_assuming_fast_index( |
There was a problem hiding this comment.
issue: please document safety invariants (even if it's obvious from the function name)
edit: it's not; because there are invariants on bit_prefix and bit_suffix.
We should document this and ensure it's upheld by the callers.
| pub unsafe fn get7(&self, ascii: u8) -> T { | ||
| debug_assert!(ascii < 128); | ||
| debug_assert!((ascii as usize) < self.data.len()); | ||
| // SAFETY: Length of `self.data` checked in the constructor. |
There was a problem hiding this comment.
issue: We may add more ctors in the future. This should reference the safety invariants on data, just say // SAFETY: Allowed by datas safety invariant, updating data's invariant to require that it has at least 128 elements and updating the constructor validation to saying something like // data safety invariant upheld here
| debug_assert!(low_six <= 0b111_111); // Safety invariant. | ||
| debug_assert!(high_five <= 0b11_111); // Safety invariant. | ||
| debug_assert!(high_five > 0b1); // Non-shortest form; not safety invariant. | ||
| // SAFETY: The highest character representable as a two-byte |
There was a problem hiding this comment.
nit: maybe introduce a newline so that this formats better
| @@ -0,0 +1,1240 @@ | |||
| // This file is part of ICU4X. For terms of use, please see the file | |||
There was a problem hiding this comment.
This file is a lot of unsafe code and I'm not convinced it is justified. Can we reduce the amount of unsafe code in this PR by writing these iterators to wrap CharIndices? There will still be unsafe in this file, but it will be around CPT invariants rather than also around UTF8 decoding.
Separately we can try and justify additional unsafe using benchmarks if needed, and that would be a nice scoped PR that can be easily reviewed and benchmarked.
There was a problem hiding this comment.
Fusing the trie lookup into UTF-8 decoding is the key point of this changeset: CPT in ICU4C has been designed so that its bit split lines up with the bits in the last UTF-8 trail byte, and we've been using it pessimally in ICU4X.
I guess I will need to port the UTF-16 NFC to NFD throughput benchmark to str and then get exact numbers for the effect here.
There was a problem hiding this comment.
Hmm, I see. I feel like using CharIndices (especially with its offset function) you might still be able to get the same benefits, but I understand if that was the point of this change.
In that case we should probably have more careful tracking of the invariant on the contained iterator whenever it is advanced.
| pub fn get8(&self, latin1: u8) -> T { | ||
| let code_point = u32::from(latin1); | ||
| debug_assert!(code_point <= SMALL_TYPE_FAST_INDEXING_MAX); | ||
| // SAFETY: `u8` is always below `SMALL_TYPE_FAST_INDEXING_MAX` and, |
There was a problem hiding this comment.
suggestion (non blocking): worth documenting on those two constants that their precise values are extremely safety relevant and relied upon by many different checks in this file
| debug_assert!(low_six <= 0b111_111); // Safety invariant. | ||
| debug_assert!(high_five <= 0b11_111); // Safety invariant. | ||
| debug_assert!(high_five > 0b1); // Non-shortest form; not safety invariant. | ||
| // SAFETY: The highest character representable as a two-byte |
There was a problem hiding this comment.
issue: the safety invariants on this function are not currently documented, but once they are, this comment should be in terms of those invariants
| /// | ||
| /// `low_six` must not have bit positions other than the lowest 6 set to 1. | ||
| /// | ||
| /// # Intended Invariant |
There was a problem hiding this comment.
question: what is this? Is this a non-safety-relevant invariant?
Perhaps explicitly say it is non-safety relevant.
| /// sequence. | ||
| #[inline(always)] | ||
| #[allow(clippy::unusual_byte_groupings)] | ||
| pub unsafe fn get_utf8_three_byte(&self, high_ten: u32, low_six: u32) -> T { |
There was a problem hiding this comment.
review progress note: this is as far as I've gotten with the thorough review, haven't yet reviewed this body.
| /// Method naming intentionally differs from the method naming on | ||
| /// those types in order to disambiguate. | ||
| #[allow(private_bounds)] // Permit sealing | ||
| pub trait AbstractCodePointTrie<'trie, T: TrieValue>: Seal { |
There was a problem hiding this comment.
nit: I think we call it Sealed in ICU4X, but it doesn't really matter
Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>
3 participants
Split out of #7526 and #7600. The code here needs to be published to crates.io, before those changes can land, because the
utf8_iterandutf16_itercrates need to depend on a versionicu_collectionsthat has this code on crates.io.Changelog
icu_collectionsnot depend onutf8_iterto avoid a circular dependency.CodePointTriegetters for fusing lookup into iterating over text: getters by Latin1, ASCII, two-byte UTF-8, and three-byte UTF-8.CodePointTries.WithTrietrait for obtaining the trie referenced by an iterator that iterates overcharandTrieValuepairs.charandTrieValuepairs for Latin1,str, and delegate iterator overchar.strthat returnsTrieValue::default()for ASCII instead reading from the trie.