ZeroTrie: Add cursor type for manual iteration and use it in BlobSchemaV2 #4383

sffc · 2023-11-30T05:57:27Z

Depends on #4381
Depends on #4382

sffc · 2023-11-30T05:57:53Z

11% improvement on BlobDataProvider read performance:

provider/construct/v1   time:   [50.531 ns 50.638 ns 50.778 ns]
                        change: [+0.0986% +0.5478% +1.0605%] (p = 0.03 < 0.05)
                        Change within noise threshold.
Found 13 outliers among 100 measurements (13.00%)
  3 (3.00%) high mild
  10 (10.00%) high severe

provider/construct/v2   time:   [50.346 ns 50.397 ns 50.451 ns]
                        change: [-0.3096% -0.0285% +0.2176%] (p = 0.83 > 0.05)
                        No change in performance detected.
Found 7 outliers among 100 measurements (7.00%)
  5 (5.00%) high mild
  2 (2.00%) high severe

provider/read/v1        time:   [1.1265 µs 1.1279 µs 1.1297 µs]
                        change: [-0.5655% -0.3793% -0.1707%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 10 outliers among 100 measurements (10.00%)
  4 (4.00%) high mild
  6 (6.00%) high severe

provider/read/v2        time:   [688.62 ns 689.91 ns 691.28 ns]
                        change: [-11.527% -11.363% -11.201%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
  7 (7.00%) high mild
  2 (2.00%) high severe

experimental/zerotrie/src/zerotrie.rs

sffc · 2023-11-30T06:04:39Z

@younies This provides the core functionality we need to implement strip_prefix but I'll do that in the next PR.

robertbastian · 2023-11-30T14:38:14Z

provider/blob/src/blob_schema.rs

+    trie: ZeroTrieSimpleAscii<&'a [u8]>,
+}
+
+impl<'a> fmt::Write for ZeroTrieStepWrite<'a> {


If you make this a general fmt::Write instead of something more locale-specific, it should handle UTF-8 input correctly. write_str currently forwards non-ASCII bytes, and write_char panics (debug) or forwards (release).

You can use the Err case to signal that non-ASCII is encountered. Then you can allow the unwrap on the grounds that locale.write_to produces ASCII only.

You should probably call this ZeroTrieSimpleAsciiStepWrite.

experimental/zerotrie/src/zerotrie.rs

robertbastian · 2023-11-30T14:45:22Z

experimental/zerotrie/src/zerotrie.rs

+    /// assert_eq!(it.head_value(), None); // "abcdxy"
+    /// ```
+    #[inline]
+    pub fn step(&mut self, byte: u8) {


Why are you keeping the lookup state in the trie itself? The API I'd expect would be:

call lookup_write(&self) to obtain a ZeroTrieSimpleAsciiLookup<'a> type that has a non-exclusive ref to the trie

that type can implement fmt::Write, or expose step

that type has a current() getter

ZeroTrieSimpleAscii is capable of stepping itself without the need to introduce any new types. This is an API that we can add regardless of whether we expose a more general ZeroTrieLookup type that has an API similar to the one you describe.

I agree that your proposed API is more intuitive and self-explanatory. If you're okay with that then I can push to this PR to implement it.

sffc · 2023-12-01T07:46:32Z

No appreciable difference in performance. Ready for re-review with the new API.

FYI @younies I added an example for how to query for the longest prefix:
http://icu4x-pr-artifacts.storage.googleapis.com/gha/d5e9e2e86fcd6f11a355e73caafcc0a992be1551/docs/zerotrie/struct.ZeroTrieSimpleAscii.html#method.cursor

robertbastian · 2023-12-01T09:52:37Z

experimental/zerotrie/src/cursor.rs

+        step_bsearch_only(&mut self.trie.store, byte)
+    }
+
+    /// Takes the value at the current position and moves the cursor.


it's unclear to me where it moves the cursor, I don't see a default place where it could be moved

I deleted peek_value which means I no longer need that clause to distinguish the behavior of value from peek_value.

Ok, but why does this "take" and use a &mut? This should not mutate.

Values are stored in trie nodes. When we read a value, we can step over that trie node so that we don't need to do that again next time we call step with the next character.

Is value() idempotent?

pub fn value(self) -> Result<usize, Self>?

I could make a moving pub fn value(self) along with a borrowing pub fn has_value(&self) which is mostly as efficient as pub fn take_value(&mut self) but I still prefer the mutating value function.

pub fn value(self) -> Result<usize, Self>?

That signature doesn't allow checking the presence of a value and then continuing, as we need to do in the longest-prefix example.

Actually my has_value suggestion does not work well in the longest prefix example if you care about retaining the value, which we probably do most of the time.

I changed the function name to take_value

robertbastian · 2023-12-01T09:54:26Z

experimental/zerotrie/src/cursor.rs

+
+impl<'a> ZeroTrieSimpleAsciiCursor<'a> {
+    /// Steps the cursor one byte into the trie.
+    ///


how does this handle non-ASCII?

provider/blob/src/blob_schema.rs

Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>

sffc · 2023-12-01T19:40:13Z

Also can someone review the diffbase PR #4382?

sffc · 2023-12-01T20:30:41Z

@younies I marked this PR as fixing #4379 because it makes it easy to do the prefix match in userland, as the example demonstrates.

dpulls · 2023-12-01T21:15:17Z

🎉 All dependencies have been resolved !

Manishearth · 2023-12-04T21:52:24Z

experimental/zerotrie/src/reader.rs

@@ -504,6 +504,80 @@ pub fn get_phf_extended(mut trie: &[u8], mut ascii: &[u8]) -> Option<usize> {
    }
 }

+pub(crate) fn step_bsearch_only(trie: &mut &[u8], c: u8) {


robertbastian · 2023-12-12T10:46:18Z

changelog pls

#4383

sffc added 7 commits November 29, 2023 23:18

Add debug_unwrap helper macro

47ca9c1

Clean up varint functions to not double assert

92a4207

Change debug_split_at to return a value in the GIGO case

512ebc6

Refactor helpers to be trait-added functions

ed19daa

Add as_borrowed_slice and AsRef impl. Need to bikeshed the name.

3743661

Initial implementation of ZeroTrie step function

46316a0

Use ZeroTrie stepping in BlobSchemaV2 to avoid allocations

2d8d8f9

sffc requested review from Manishearth and a team as code owners November 30, 2023 05:57

sffc commented Nov 30, 2023

View reviewed changes

experimental/zerotrie/src/zerotrie.rs Outdated Show resolved Hide resolved

sffc requested a review from younies November 30, 2023 06:04

sffc requested a review from robertbastian November 30, 2023 06:04

robertbastian reviewed Nov 30, 2023

View reviewed changes

younies mentioned this pull request Nov 30, 2023

Implement MeasureUnit #4360

Merged

sffc added 11 commits November 30, 2023 13:05

Merge branch 'main' into zerotrie-step

f272f0e

Add APIs for ZeroTrieSimpleAsciiCursor and use them in BlobSchemaV2

f9a614e

Move around examples and cargo fmt

929f81f

Return core::fmt::Error instead of asserting ascii

79fc890

Move the core::fmt::Write impl into the zerotrie crate

3a8efa1

Don't assert unreachable anymore

838ad05

Docs for the new core::fmt::Write impl

4e70618

Move cursor impls to their own file

8f754cc

fmt, clippy

bb4d1e7

Delete the .head_value() function in favor of .cursor().value()

babaf98

Change .value() to a mutating function and add .peek_value()

d5e9e2e

sffc requested a review from robertbastian December 1, 2023 07:44

robertbastian reviewed Dec 1, 2023

View reviewed changes

robertbastian mentioned this pull request Dec 1, 2023

Remove IanaToBcp47MapperBorrowed::get_bytes #4398

Merged

sffc and others added 2 commits December 1, 2023 13:24

Update provider/blob/src/blob_schema.rs

fb05eb0

Co-authored-by: Robert Bastian <4706271+robertbastian@users.noreply.github.com>

Remove peek_value and impl Clone instead

5ce1b16

sffc requested a review from robertbastian December 1, 2023 19:39

sffc added 2 commits December 1, 2023 13:15

Merge branch 'main' into zerotrie-step

eeb3cbe

value -> take_value

7017868

younies previously approved these changes Dec 4, 2023

View reviewed changes

Manishearth previously approved these changes Dec 4, 2023

View reviewed changes

Docs for internal functions

09b56c0

sffc dismissed stale reviews from Manishearth and younies via 09b56c0 December 4, 2023 21:59

Manishearth approved these changes Dec 4, 2023

View reviewed changes

sffc changed the title ~~ZeroTrie: Add step function for manual iteration and use it in BlobSchemaV2~~ ZeroTrie: Add cursor type for manual iteration and use it in BlobSchemaV2 Dec 4, 2023

robertbastian approved these changes Dec 5, 2023

View reviewed changes

robertbastian merged commit f50e87f into unicode-org:main Dec 5, 2023
29 checks passed

sffc deleted the zerotrie-step branch December 5, 2023 17:18

sffc added a commit that referenced this pull request Dec 27, 2023

Update CHANGELOG.md

1bac29f

#4383

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZeroTrie: Add cursor type for manual iteration and use it in BlobSchemaV2 #4383

ZeroTrie: Add cursor type for manual iteration and use it in BlobSchemaV2 #4383

sffc commented Nov 30, 2023 •

edited

Loading

sffc commented Nov 30, 2023

sffc commented Nov 30, 2023

robertbastian Nov 30, 2023

robertbastian Nov 30, 2023

sffc Nov 30, 2023

sffc Nov 30, 2023

robertbastian Nov 30, 2023

sffc Dec 1, 2023

sffc commented Dec 1, 2023

robertbastian Dec 1, 2023

sffc Dec 1, 2023 •

edited

Loading

robertbastian Dec 1, 2023

sffc Dec 1, 2023

robertbastian Dec 1, 2023

robertbastian Dec 1, 2023

sffc Dec 1, 2023

sffc Dec 1, 2023

sffc Dec 1, 2023

sffc Dec 2, 2023

robertbastian Dec 1, 2023

sffc Dec 1, 2023

sffc commented Dec 1, 2023

sffc commented Dec 1, 2023

dpulls bot commented Dec 1, 2023

Manishearth Dec 4, 2023

robertbastian commented Dec 12, 2023

ZeroTrie: Add cursor type for manual iteration and use it in BlobSchemaV2 #4383

ZeroTrie: Add cursor type for manual iteration and use it in BlobSchemaV2 #4383

Conversation

sffc commented Nov 30, 2023 • edited Loading

sffc commented Nov 30, 2023

sffc commented Nov 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Dec 1, 2023

Choose a reason for hiding this comment

sffc Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sffc commented Dec 1, 2023

sffc commented Dec 1, 2023

dpulls bot commented Dec 1, 2023

Choose a reason for hiding this comment

robertbastian commented Dec 12, 2023

sffc commented Nov 30, 2023 •

edited

Loading

sffc Dec 1, 2023 •

edited

Loading