perf(regex): answer a plain-string split without the engine - #10816
proggeramlug wants to merge 1 commit into
Conversation
Linking `regex-engine` replaces `String.prototype.split` wholesale (see
`string::mod`), so a program that uses a regex anywhere ran every split through
the engine's per-UTF-16-unit subject reader -- even `"a b".split(" ")`, where the
engine has nothing to contribute. Measured on one source compiled twice:
split(" "), engine linked 37,558 instructions
split(" "), engine absent 3,662
node 26.5.1 2,714
Both arms auto-optimized, so that is the implementation swap rather than the
build mode; specialization accounts for about 5% of it. Roughly 48% of the
engine path is `Units::at`, `BoundSpan::retarget`, `Cursor::next_unit` and
`copy_units`.
The plain algorithm now answers the call when it provably agrees, below the
`@@split` check so a custom splitter still wins. Three input classes are
excluded rather than repaired, because the two implementations genuinely differ
on them:
* a separator holding a lone surrogate, which the engine matches against one
half of a valid pair and a WTF-8 byte scan cannot;
* a separator that is not already a string, whose ToString can run user code
or throw -- a Symbol must raise TypeError;
* a `limit` that is not already a number or undefined, whose ToNumber can
throw.
Everything excluded takes the engine path, so this only narrows what the fast
path answers. The plain algorithm also reports failure by throwing where this
module returns Err, so the call is wrapped in `api::caught`.
split(" "): 37,558 -> 5,081 instructions, -86.5%, 13.84x -> 1.87x node
Answers are identical to Node on 27 cases where a byte scan and a unit scan can
disagree -- empty separator, separator longer than the subject, every `limit`
form, lone surrogates, an astral pair split by units, a separator that is a
prefix of itself at the tail -- and on 12 non-string separator forms including
`@@split` callable and not callable.
Claude-Session: https://claude.ai/code/session_018M47oWitg2Hf1jzfhLqgQ9
|
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 (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe regex split entry point now delegates eligible inputs to the plain string split implementation after checking ChangesPlain split delegation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Refactor Sequence Diagram(s)sequenceDiagram
participant StringSplit
participant SymbolSplit
participant PlainSplit
participant RegexEngine
StringSplit->>SymbolSplit: check @@split
SymbolSplit-->>StringSplit: no custom split
StringSplit->>PlainSplit: split plain separator and limit
StringSplit->>RegexEngine: process non-plain split
🚥 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 242 (#10830) as v0.5.1621 — Eight PRs travelled together because their file sets are disjoint — 30 files, +1,514/−101, zero overlap. Validated as one tree: ten cheap gates, Two of the eight needed a fix before they could land, both made in the train rather than bounced back. #10816 bound #10817 added 15 dispatch entries without regenerating the docs, so the API-docs-drift check failed. Regenerated from a built binary: 2855 → 2870, exactly your 15, with For future PRs in this area: One more thing, aimed at whoever cuts the next PR here: |
1 participant
Linking
regex-enginereplacesString.prototype.splitwholesale, so a program using a regex anywhere ran every split through the engine's per-UTF-16-unit subject reader — even"a b".split(" "), where the engine has nothing to contribute.One source compiled twice, loop-variant receivers, control-subtracted:
Both arms auto-optimized, so this is the implementation swap and not the build mode — specialization is ~5% of it. ~48% of the engine path is
Units::at,BoundSpan::retarget,Cursor::next_unit,copy_units.Result: 37,558 → 5,081 instructions, −86.5%, 13.84× → 1.87× node.
What it does
The plain algorithm answers the call when it provably agrees, placed below the
@@splitcheck so a custom splitter still wins.Three input classes are excluded rather than repaired, because the implementations genuinely differ — each was found by a failing test, not by inspection:
"😀😀".split(lowHalf)is 3 parts, not 1);ToStringcan run user code or throw — a Symbol must raiseTypeError;limitnot already a number orundefined, whoseToNumbercan throw (BigInt, boxedvalueOf).Everything excluded takes the engine path, so this only narrows what the fast path answers. The plain algorithm also reports failure by throwing where this module returns
Err, so the call is wrapped inapi::caught.Evidence
limitform, lone surrogates, an astral pair split by units, a separator that is a prefix of itself at the tail, overlapping separators — identical to Node.@@splitcallable and not callable — identical to Node.perry-runtimelib suite 4100 passed, 0 failed;--lockedbuild, fmt,-D warnings(regex-off and product), GC root holders, file size, release build all OK.Lint gates: 2 of 85 fail, both pre-existing on pristine main and unreachable from this diff.
Worth knowing separately
The excluded classes exist because
split::— whatnot(regex-engine)builds ship — diverges from the engine on@@split, surrogate half-pairs, uint32 limits and primitive hooks. Those builds have carried that all along; nothing tests it, because the default feature set never compiles it. Not fixed here.https://claude.ai/code/session_018M47oWitg2Hf1jzfhLqgQ9
Summary by CodeRabbit
String.prototype.splithandling for plain string separators and non-coercive limits.Symbol.split.