perf(string): scan for ASCII a word at a time, and memchr a one-byte split - #10910
proggeramlug wants to merge 1 commit into
Conversation
…split Two per-call costs in `js_string_split_n`, both measured on PerryTS#10519's own reproduction -- a 211-byte JWT split on "." -- after the engine path stopped answering plain-string splits. The source is scanned once per call to decide whether every part can take the all-ASCII metadata shortcut. That scan was `bytes.iter().all(|&b| b < 0x80)`, one byte per iteration; `perf annotate` put about 74% of this function's own time in its six-instruction loop, roughly 19% of the whole call. `is_ascii()` is the same predicate and std tests a word at a time. The parts are then found with `str::split(&str)`, which takes the two-way `StrSearcher` and pays its setup in full for a single-character needle -- the common `split(".")`, `split(",")`, `split(" ")` shape. A one-byte delimiter takes std's memchr-based searcher instead. The `byte < 0x80` guard on that second path is load-bearing rather than decorative: string storage can hold malformed bytes (the comment above the ASCII scan documents `[0x80, b'|', 0xF0]`), and `0x80 as char` is U+0080, which re-encodes as two bytes and would split on the wrong occurrences. JWT split on ".": 5,424 -> 4,268 instructions per call, -21.3% Against PerryTS#10519's 345x and ~230k instructions, that row is now 9.2x Node 26.5.1. Split conformance is unchanged: the 27 cases where a byte scan and a UTF-16 unit scan can disagree, and the 12 non-string separator forms, all still match Node.
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe string split implementation now uses direct ASCII detection and selects character-based splitting for non-empty ASCII delimiters. Non-ASCII and malformed-byte delimiters retain string-pattern splitting. ChangesString Split ASCII Handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Landed via merge train 253 (#10918) as v0.5.1633 — merge commit Expedited at the owner's request. Carried with nine other PRs; the stacked ones (#10899/#10900 on #10886, #10901 on #10885) had only their unique commits taken. Evidence on the assembled tree: Closing here rather than merging — a train lands the commits directly. |
1 participant
Two per-call costs in
js_string_split_n, both measured on #10519's own reproduction — a 211-byte JWT split on"."— now that the engine path no longer answers plain-string splits (#10816).5,424 → 4,268 instructions per call, −21.3%. That row is now 9.2× node, against the 345× and ~230k instructions #10519 records.
The ASCII scan
The source is scanned once per call to decide whether every part can take the all-ASCII metadata shortcut. That scan was
bytes.iter().all(|&b| b < 0x80)— one byte per iteration.perf annotateput ~74% of this function's own time in its six-instruction loop, roughly 19% of the entire call:is_ascii()is the same predicate and std tests a word at a time.The one-byte delimiter
Parts were found with
str::split(&str), which takes the two-wayStrSearcherand pays its setup in full for a single-character needle — the commonsplit("."),split(","),split(" ")shape. A one-byte delimiter now takes std's memchr-basedcharsearcher.The
byte < 0x80guard on that path is load-bearing, not decorative. String storage can hold malformed bytes — the comment above the ASCII scan documents[0x80, b'|', 0xF0]— and0x80 as charis U+0080, which re-encodes as two bytes and would split on the wrong occurrences. "One byte implies ASCII" only holds for well-formed input.Evidence
Split conformance unchanged: the 27 cases where a byte scan and a UTF-16 unit scan can disagree (empty separator, separator longer than the subject, every
limitform, lone surrogates, an astral pair split by units, a separator that is a prefix of itself at the tail) and the 12 non-string separator forms all still match Node.perry-runtimelib suite 4200 passed, 0 failed;--lockedbuild, fmt,-D warnings, GC holder audit, file size and the release build all OK. Lint gates: 1 of 85 fails, "Public benchmark evidence freshness", pre-existing on main.What is left on #10519
Roughly, from the profile: the per-part write barrier ~5.6%, the result-array allocation ~3.5%, a heap
Vecof part ranges per call ~2.6%, and ~6% from the admission check #10816 added (api::caughtplus aToStringon the separator). None is a single dominant term the way the ASCII scan was.Summary by CodeRabbit