Repository navigation
BPETokenizer: build vocab and merge strings natively to speed up load - #416
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…ge ranks BinaryDistinctString.string went through a bridged NSString and init(NSString) split into grapheme clusters only to re-join their UTF-16, so every vocab key and merge half reached the BPE dictionaries as a foreign String and took the slow hashing and comparison path. Build the String natively with String(decoding:as:UTF16.self), take utf16 directly in the initializers, and reserve capacity for bpeRanks. A lone surrogate keeps its exact NSString (nsString falls back to the old construction when the UTF-16 is not well formed).
3a0299b to
99afeac
Compare
pcuenca
left a comment
There was a problem hiding this comment.
Thanks, this looks good. However, the lone surrogate behaviour differs on Linux. I verified locally using this test, which for some reason I can't submit to your PR.
Could you please accept that commit into your branch so we can run CI here to see how the lone surrogate tests fail? Then we can maybe just run them on Darwin, or fix them so they pass on Linux too.
Run new BinaryDistinctStringTests on Linux
swift-corelibs-foundation stores an NSString as a Swift String, so String(utf16CodeUnits:count:) repairs a lone surrogate to U+FFFD there, with the previous implementation as with this one. Assert the Darwin behaviour on Darwin and the repaired form elsewhere. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks! I've merged your commit. The failures are in the test's expectations, and not a behaviour change. On Linux, swift-corelibs-foundation stores an NSString as a Swift String, and String(utf16CodeUnits:count:) decodes with repair, so a lone surrogate always became U+FFFD there, before this PR too. The pre-PR code fails the same 7 assertions on Linux. I've made those two assertions platform-specific (Darwin keeps the lone surrogate, Linux gets U+FFFD), so the test now passes on Linux against both the old and the new code. PTAL. |
While profiling model load in an app that uses swift-transformers, I found that
AutoTokenizer.from(modelFolder:)is most of the wall time for large BPE vocabularies. Gemma 4'stokenizer.json(31 MB, 262,144 vocab entries, 514,906 merges) takes about 1.4 s on an M-series Mac in a Release build, and about 2.9 s in Debug. This PR cuts that by roughly 2.3x without changing the loaded data. It is a small change, and I'm happy to rework it however you prefer.What the profile showed (Time Profiler, Release): about 64% of the time is in
BPETokenizer.init, mostly buildingbpeRanks.BinaryDistinctString.stringwasString(nsString)wherensStringisString(utf16CodeUnits:count:) as NSString. Every vocab key and merge half therefore reachedbpeRanksand the token dictionaries as a bridged (foreign) String, so each hash and==went through the UTF-16-via-CoreFoundation path, and non-ASCII keys also paid for normalization in the hash.BinaryDistinctString.init(_: NSString)wasArray(str as String).flatMap { $0.utf16 }, which splits the string into grapheme clusters only to concatenate their UTF-16 again.bpeRanksgrew from empty to about 515k entries with noreserveCapacity.Changes:
BinaryDistinctString.stringis nowString(decoding: value, as: UTF16.self)(a native String).init(_: NSString)andinit(_: String)take.utf16directly.nsStringreturnsstring as NSStringwhen the UTF-16 is well formed, and falls back to the previous construction otherwise. This keeps lone surrogates exactly as before:String(decoding:)would turn one into U+FFFD, which would change which NSString a vocabulary key is. (A lone\ud800escape is rejected by yyjson, so atokenizer.jsoncannot carry one, but aConfigbuilt in code can.)BPETokenizer.initcallsbpeRanks.reserveCapacity(merges.count).Measurements:
AutoTokenizer.from(modelFolder:)on an Apple Silicon Mac, median of 5 timed runs after one warm-up, base = 1.3.5, interleaved runs:Some Debug baseline runs were noisy because other jobs were running on the machine; the PR numbers were stable.
Behaviour: I encoded a multilingual probe (accents, composed and decomposed forms, ZWJ emoji, Hangul, CJK, Arabic, a ligature) with both builds for both tokenizers and the ids are identical (same FNV digest of the id sequence). There is a new test,
BinaryDistinctStringTests, inTokenizersTests. It keeps the old implementations as a reference and checks that the UTF-16 values,stringview andnsStringview match for well-formed strings (composed and decomposed forms stay distinct) and for lone surrogates. It also builds aBPETokenizerfrom JSON and from aConfigcontaining a lone-surrogate vocabulary key, and checks the vocabulary and merge ranks are unchanged. The test passes on both the old and the new code. The package tests pass locally (I skipped theBenchmarkstarget; one test was already skipped).Something I did not do: reading
model.vocabandmodel.mergesstraight from the yyjson document into native dictionaries, without going throughConfig, would likely save more (the yyjson-to-Configconversion andmergesFromConfigwere about 40% of the profile before this change). That is a bigger change, so I kept this PR to the part that doesn't touch the parsing structure. If that direction interests you I can try it separately.