Yarr: guard class-set string disjunctions with a first-character prefilter - #401
Yarr: guard class-set string disjunctions with a first-character prefilter#401robobun wants to merge 1 commit into
Conversation
…ilter
A class set with strings (e.g. /\p{RGI_Emoji}/v) expands into one
alternative per string. RGI_Emoji has ~3700 strings, so every failing
match attempt walked all of those alternatives: matching a non-emoji
character was ~300x slower than V8, which made string-width (and any
Ink-based TUI) unusable on non-ASCII output.
Wrap the expanded string alternatives in a lookahead on the set of
possible first characters:
(?: s1 | ... | sN | [singles] )
becomes
(?: (?=[firstChars]) (?: s1 | ... | sN ) | [singles] )
A non-matching input now fails after one character-class check (binary
search) instead of trying every string alternative, in both the Yarr
interpreter and JIT. Input that cannot start any string but is in the
single-character set skips straight to the trailing class alternative.
The prefilter is skipped for backward matching (inside lookbehinds),
for sets containing the empty string (it matches without consuming, so
no first character exists), and for small sets (< 16 strings) where the
extra lookahead does not pay for itself and where the anchored
string-list fast path would otherwise apply.
On /^\p{RGI_Emoji}$/v.test(non-emoji) x 156 distinct strings
(release, linux x64):
before: 3.68 ms/iter (~1170x the \p{Extended_Pictographic} control)
after: 0.008 ms/iter (~1.8x the control, ~2x faster than Node 26)
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it rewrites how Yarr expands v-mode class sets into pattern terms — a correctness-critical path in the regex engine — a human look would still be worthwhile.
What was reviewed:
- Verified the rewrite
(?: s1|…|sN | [singles] )→(?: (?=[firstChars])(?: s1|…|sN ) | [singles] )is semantically equivalent, including under backtracking. - Checked
m_stringsisVector<Vector<char32_t>>, sostring[0]passed toputCharis a full codepoint (not a lone surrogate) for non-BMP emoji. - Confirmed the guard conditions (backward match, empty string, <16 strings, invert) all fall back to the original expansion with no prefilter.
- Checked ownership: the prefilter
CharacterClassis appended tom_userCharacterClassesbefore its raw pointer is used, and the early-return-on-empty-string path just drops the local constructor.
Extended reasoning...
Overview
This PR modifies YarrPattern.cpp to optimize v-mode regex class sets that contain strings (e.g. /\p{RGI_Emoji}/v). It extracts the two duplicated expansion loops (in atomBuiltInCharacterClass and atomCharacterClassEnd) into a shared helper atomClassSetWithStringsExpansion, and adds a new stringDisjunctionPrefilter that builds a character class of possible first characters. When applicable, the string alternatives are wrapped in (?=[firstChars])(?:...) so a non-matching input fails after one class check instead of walking thousands of alternatives.
Security risks
None identified. This is a pattern-compile-time transformation inside the Yarr regex compiler; it doesn't touch parsing of untrusted input in a new way, doesn't change memory ownership patterns beyond appending one more CharacterClass to the existing m_userCharacterClasses owning vector, and doesn't introduce new bounds-unchecked accesses (string[0] is guarded by string.isEmpty()).
Level of scrutiny
High. Yarr pattern construction feeds both the interpreter and the JIT, and a subtle mistake here would produce incorrect regex results across all of Bun rather than a crash. The transformation is a semantic rewrite of the compiled pattern, so it needs to be equivalence-preserving under all backtracking scenarios, quantifiers applied to the group, capture semantics, and interaction with the surrounding pattern. The PR description reports extensive testing (stress tests on debug+ASAN, a correctness suite cross-checked against Node covering quantifiers/lookbehinds/set-subtraction/ignore-case), which is reassuring, but the change is intricate enough that a maintainer familiar with Yarr's parenthesis-context stack and terminal-parentheses fast path should sign off.
Other factors
- The refactor is clean: the no-prefilter path through
atomClassSetWithStringsExpansionis byte-for-byte the original loop, so behaviour is unchanged when the guard conditions reject the optimization. - The
invertguard is defensive — both call sites already prevent inverted class sets with strings from reaching this code (one viaErrorCode::NegatedClassSetMayContainStrings, the other because\P{}on properties of strings is a v-mode syntax error), but skipping the prefilter oninvertis harmless. - I checked that
atomParentheticalAssertionBegin(false, Forward)/atomParenthesesEnd()pairs balance correctly with the outer and inneratomParenthesesSubpatternBegin(false)calls in both the with-singles and without-singles branches. - No CODEOWNERS file governs this path, but Yarr changes in this fork typically warrant a look from someone who tracks upstream WebKit's regex engine.
Preview Builds
|
|
Closing: #299 landed first and fixes the same case (a class of strings is now dispatched on the input's first code point), and Bun picked it up in oven-sh/bun#37352; oven-sh/bun#37290 is closed and oven-sh/bun#37294, which pinned this branch, is closed as superseded. |
Fixes the JSC side of oven-sh/bun#37290.
Problem
A v-mode class set with strings (
/\p{RGI_Emoji}/v) expands into one flat alternative per string. RGI_Emoji has ~3700 strings, so every failing match attempt walks all of those alternatives./^\p{RGI_Emoji}$/v.test()on a non-emoji character measured ~300x slower than V8. string-width@8 runs this regex per grapheme cluster, so any Ink/TUI output containing box-drawing characters spent almost all of its CPU here (the bun issue measured 84% of process CPU in this one regex).Fix
YarrPattern.cpp: when expanding a class set with strings, wrap the string alternatives in a lookahead on the set of possible first characters:A non-matching input now fails after one character-class check (binary search) in both the interpreter and the JIT, and input that cannot start any string but is in the single-character set skips straight to the trailing class alternative.
The prefilter is skipped when:
checkForTerminalParentheses) would otherwise apply.Built-in properties of strings always pass through this (RGI_Emoji, Basic_Emoji, the RGI_Emoji_*_Sequence classes); large user
\q{...}disjunctions do too.Numbers
/^\p{RGI_Emoji}$/v.test()over 156 distinct single-character strings, release JSCOnly, linux x64:\p{Extended_Pictographic}controlThe matching-input case also improves (3.09 -> 1.78 ms/iter) but still walks the string alternatives; making hits fast would need a first-character dispatch across alternatives (trie), which is left as future work.
Testing
JSTests/stress/regexp-vflag-property-of-strings.jsand the surrounding regexp/yarr stress tests pass on a debug+ASAN build (yarr-terminal-parentheses-frame-layout.jstakes 5m19s patched vs 5m18s unpatched, both pass).\qdisjunctions (with and without empty string), ignore-case, and lookbehinds: matches Node on all cases (one pre-existing, unrelated lookbehind failure/(?<=\p{RGI_Emoji})x/v.test('😀x')is wrong both before and after this change).