perf(merge bpe): improvement suggestions by SBrandeis · Pull Request #2275 · huggingface/tokenizers

SBrandeis · 2026-08-03T18:16:02Z

No description provided.

HuggingFaceDocBuilderDev · 2026-08-03T18:19:46Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

- Use a trait generic instead of a const generic - 2 Specialized struct instead of mangling all into SymbolSink - fn that had no reason to live in impl PipelineBpe are moved oout

ArthurZucker

Thanks a lot for the great PR 🤗

ArthurZucker · 2026-08-04T01:01:21Z

+//! A pass builds the new word in the same array that holds the old one, using two cursors that both start at index 0.
+//! The read cursor marks the start of what is left of the old word.
+//! The write cursor marks the end of the new word built so far.
+//! Every step writes exactly one symbol: a copy moves both cursors by one, and a merge writes one symbol but consumes two, so the read cursor moves ahead.


Suggested change

//! Every step writes exactly one symbol: a copy moves both cursors by one, and a merge writes one symbol but consumes two, so the read cursor moves ahead.

//! Every step writes exactly one symbol: a copy (a no merge for a pair just results in copying the id that was just read) moves both cursors by one, and a merge writes one symbol but consumes two, so the read cursor moves by 2.

ArthurZucker · 2026-08-04T01:01:35Z

+//! The read cursor marks the start of what is left of the old word.
+//! The write cursor marks the end of the new word built so far.
+//! Every step writes exactly one symbol: a copy moves both cursors by one, and a merge writes one symbol but consumes two, so the read cursor moves ahead.
+//! The write cursor never gets ahead of the read cursor, so a write only ever lands on a slot that was already read.


Suggested change

//! The write cursor never gets ahead of the read cursor, so a write only ever lands on a slot that was already read.

ArthurZucker · 2026-08-04T01:02:15Z

+//!   ┌───┬───┬────┬───┐
+//!   │ h │ e │ ll │ o │
+//!   └───┴───┴────┴───┘


I would leave the last slot since it will just get a u32::MAX

ArthurZucker · 2026-08-04T01:02:44Z

+//!   pass 2  target (h,e):     [ h │ e │ ll │ o ]   ->   [ he │ ll │ o ]   lowest written pair: (ll,o)
+//!   pass 3  target (ll,o):    [ he │ ll │ o ]      ->   [ he │ llo ]      lowest written pair: (he,llo)
+//!   pass 4  target (he,llo):  [ he │ llo ]         ->   [ hello ]         no pair left to rank: done


same here! Its a nit but important IMO to show that there is "padding"

ArthurZucker · 2026-08-04T01:04:13Z

+//!
+//! # Batching and the `SAFE` bit
+//!
+//! The target can occur several times in the word. When its merge is `SAFE`, one pass merges every occurrence.


Suggested change

//! The target can occur several times in the word. When its merge is `SAFE`, one pass merges every occurrence.

//! The target merge can occur several times in the word (for example "hello lots", the pair "lo" appears twice). When its merge is `SAFE`, one pass merges every occurrence.

ArthurZucker · 2026-08-04T02:11:47Z

+            write_cursor: 0,
+            target_merge,
+            batched,
+            has_merged: false,


was_merged?

ArthurZucker · 2026-08-04T02:12:41Z

+    /// Looks up the pair at the read cursor and writes one symbol: the pair's product id when
+    /// its value equals the target and the pair may still merge, the left symbol otherwise. A
+    /// merge consumes both symbols of the pair, a copy only the left one.
    ///
-    /// `&mut [u32]` rather than `&mut Vec<u32>` so the length is a local and the reads can have
-    /// their bounds checks removed..
+    /// The return value becomes the next call's `known_pair_value`. After two copies in a row,
+    /// the pair the second write ranks is the (`left_symbol`, `right_symbol`) the first call
+    /// already looked up, so the second write reuses that value instead of looking it up again.
+    /// A merge returns `None`: the product id it writes is a new symbol, and no pair containing
+    /// it has been looked up yet.


badly formulated, but yes you are winning one lookup! let's put this in better framing!

ArthurZucker · 2026-08-04T02:15:58Z

+        known_pair_value = state.step(tables, symbols, known_pair_value);
+    }
+    if state.read_cursor < len {
+        state.copy_last(tables, symbols, known_pair_value);


not sure we need the copy last func but nit

ArthurZucker · 2026-08-04T02:16:46Z

+        let pair_value = tables.get_value(&left_symbol, &right_symbol);
+        let should_merge = pair_value == self.target_merge && (self.batched || !self.has_merged);
+        if should_merge {
+            self.has_merged = true;


Suggested change

self.has_merged = true;

self.was_merged = true;

ArthurZucker · 2026-08-04T02:20:54Z

-            let merge_rank = self.tables.get_value(&to_merge[write_id - 1], &written);
-            running_min = std::cmp::min(running_min, merge_rank);
+            self.read_cursor += 1;
+            self.write(tables, symbols, left_symbol, known_pair_value);


known_paire_value naming is a bit weird / not explicit that it will just not be looked up and None will be?

Brings in the BPE merge work from #2241 and #2275: the multipass and two-tier-queue engines, the rank tables and the pretoken->rank conversion. `models/bpe/model.rs` is gone -- #2241 split it into `bpe_model.rs`, `bpe_scratch.rs` and the merge engines -- so the word cache and the batched `tokenize_spans` this branch added to it are re-wired onto the new engine: `BpeScratch` carries `word_cache` again, and both pipeline entry points emit through `tables.unmap` instead of `Word::get_chars_iter`.

SBrandeis added 4 commits August 3, 2026 19:34

rewrite + document multipass

2624c43

rename

db1798d

perf: save a lookup when there is no merge

7598fb2

perf: compiler gotchas

ff4d576

Idiomacy:

e9fe3ca

- Use a trait generic instead of a const generic - 2 Specialized struct instead of mangling all into SymbolSink - fn that had no reason to live in impl PipelineBpe are moved oout

SBrandeis force-pushed the perf/bpe-merge-review branch from 544c602 to e9fe3ca Compare August 3, 2026 19:13

ArthurZucker approved these changes Aug 4, 2026

View reviewed changes

ArthurZucker marked this pull request as ready for review August 4, 2026 03:58

ArthurZucker mentioned this pull request Aug 4, 2026

perf(pipeline): don't stride an input that has no cut points #2283

Open

Navigation Menu

Uh oh!

perf(merge bpe): improvement suggestions - #2275

perf(merge bpe): improvement suggestions#2275
SBrandeis wants to merge 5 commits into
perf/bpe-mergefrom
perf/bpe-merge-review

SBrandeis commented Aug 3, 2026

Uh oh!

HuggingFaceDocBuilderDev commented Aug 3, 2026

Uh oh!

ArthurZucker left a comment

Uh oh!

ArthurZucker Aug 4, 2026

Uh oh!

ArthurZucker Aug 4, 2026

Uh oh!

ArthurZucker Aug 4, 2026

Uh oh!

ArthurZucker Aug 4, 2026

Uh oh!

ArthurZucker Aug 4, 2026

Uh oh!

ArthurZucker Aug 4, 2026

Uh oh!

ArthurZucker Aug 4, 2026

Uh oh!

ArthurZucker Aug 4, 2026

Uh oh!

ArthurZucker Aug 4, 2026

Uh oh!

ArthurZucker Aug 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	//! Every step writes exactly one symbol: a copy moves both cursors by one, and a merge writes one symbol but consumes two, so the read cursor moves ahead.
	//! Every step writes exactly one symbol: a copy (a no merge for a pair just results in copying the id that was just read) moves both cursors by one, and a merge writes one symbol but consumes two, so the read cursor moves by 2.

	//! The target can occur several times in the word. When its merge is `SAFE`, one pass merges every occurrence.
	//! The target merge can occur several times in the word (for example "hello lots", the pair "lo" appears twice). When its merge is `SAFE`, one pass merges every occurrence.

Uh oh!

Conversation

SBrandeis commented Aug 3, 2026

Uh oh!

HuggingFaceDocBuilderDev commented Aug 3, 2026

Uh oh!

ArthurZucker left a comment

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants