perf(runtime): [^] and [] no longer make regex construction case-fold a million code points — cc --help −3.78% - #9216
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe regex translator now emits ChangesRegex any-character rewrite
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change avoids unnecessary case-folding work for narrowly recognized regex classes while preserving output and matching behavior in the supplied tests; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Description checkExplanation The description is detailed and on-topic. It covers the motivation, implementation, scope, performance results, and verification. It omits the template headings, an explicit Related issue value, and the checklist, but the required technical content is mostly present. Full details: Docstring CoverageExplanation Docstring coverage is 88.89% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 2 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…rms regex_syntax does not case-fold
`[\s\S]` and `(?s:.)` denote the same set, but `regex_syntax`'s HIR
translator case-folds a character class by looping over EVERY CODE POINT
in each of its ranges:
for cp in (start..=end).filter_map(char::from_u32) {
for &cp_folded in folder.mapping(cp) { ranges.push(...) }
}
`[\s\S]` canonicalizes to the single range `\x{0}-\x{10FFFF}`, so under
`(?i)` that loop runs 1,114,112 times to compute a fold that cannot
change anything — the class already contains every character. `.` under
`(?s)` is `Hir::dot`, not a class, and is never folded. The negated
mirror `[^\s\S]` is just as bad: the fold runs on the positive set,
before the negation.
`fancy_regex::Regex::new`, best of 3, same machine:
(?i)[\s\S]*? 7.72 ms (?i)(?s:.)*? 41.7 us 185x
(?i)[^\s\S] 7.46 ms (?i)[a&&b] 11.4 us 654x
and it compounds with the number of occurrences (three `[\s\S]` in one
pattern: 23.0 ms vs 34.9 us).
Why it matters: a symbolized perf profile of the claude-code bundle's
`--help` put `ClassUnicodeRange::case_fold_simple` at 3.78% of ALL
retired instructions — the second-largest symbol in the binary.
Recording a stack on every one of its 2,754 calls (uprobe + dwarf)
attributed 99.6% of them to construction-time validation, not to the
lazy first-use build PerryTS#9178 introduced; a gdb capture of the argument
named the patterns: five successive `new RegExp` refinements of marked's
`/^ {0,3}(?:<(script|pre|style|textarea)[\s>][\s\S]*?.../i` HTML-block
rule — the `i` flag plus eight `[\s\S]` — and one URL validator. A probe
on `get_or_compile_regex` (verified able to fire: it saw 3,713 calls)
shows that run never matches with any of them.
This also fixes the shape in perry's OWN output: JS `[^]` was translated
to `[\s\S]` and JS `[]` to `[^\s\S]`, so perry manufactured the
pathological form itself.
The rewrite is deliberately narrow: only a six-character class whose two
members are a shorthand and its own complement, and only outside a
class. The group is non-capturing and `collect_capture_spans` indexes
the input, so capture numbering is untouched.
Gates: near-miss unit tests (mutation-checked twice — dropping the
closing-bracket check makes `near_misses_are_left_alone` fail, and
emitting `(?s:.)` for `[]` makes the behaviour test fail), a
match-behaviour test covering the negated forms, quantifiers, capture
numbering and `.source`, the 63 existing regex tests, the full 2,855-test
perry-runtime suite, and `syntax_check_agrees_with_full_build` over
12,077 patterns (claude-code + pi + kimi + 6,297 mutations).
87c3ee9 to
2e495b0
Compare
|
Merged. The mechanism is convincing on its face —
Validation: Validated on a shared branch with five other PRs, so if anything downstream looks odd, the co-resident changes were #9213, #9214, #9219, #9224 and #9230. |
…") cuts at UTF-16 code units (#9408, #9409) (#9427) * fix(runtime): multiline ^ and $ hold at every LineTerminator (#9408) ECMAScript §22.2.2.6 defines the `m`-flag anchors over the same four characters a non-dotAll `.` excludes — LF, CR, U+2028, U+2029 — but the translation spelled multiline as Rust's `(?m)`, which recognizes LF and nothing else. `"one\rtwo".match(/^.*$/gm)` was `null`, and CRLF (TWO terminators, with an empty line between them) reported `["two"]` instead of `["one","","two"]`. Neither engine has a configurable line-terminator SET (`regex`'s `line_terminator` is a single byte), so `^`/`$` are now translated to explicit `\A`/lookbehind and `\z`/lookahead assertions built from the LineTerminator set #9218 established for the dot. One macro spells that set, so the dot and the two anchors cannot drift apart. Only patterns that actually carry `m` are rewritten; `^`/`$` inside a character class and the escaped `\^`/`\$` forms are untouched, which keeps #9216's `[^]` -> `(?s:.)` and `[]` -> `[a&&b]` rewrites firing under `m`. The lookaround is outside the linear engine's grammar, so a multiline anchored pattern now selects `fancy-regex` — the tests assert both halves of that (linear refuses, fancy accepts) because #9305 is the precedent for a translator change only one engine could parse. test-files/test_gap_9408_multiline_anchors_lineterminators.ts is byte-compared against node. Built from unfixed origin/main the same fixture diverges on 154 lines. Claude-Session: https://claude.ai/code/session_014knX724SYDogwzsXybCGxp * fix(runtime): split("") cuts at UTF-16 code units (#9409) §22.1.3.23 runs SplitMatch over the code-unit sequence, so an astral character — one WTF-8 sequence but TWO code units — is two parts, each a lone surrogate. Perry stepped its payload one WTF-8 sequence per part, so `"😀".split("")` had length 1 while `"😀".length` was already 2 and `charAt(0)`/`charAt(1)` already returned the halves: the iterator disagreed with the representation, not with itself. The count pass now adds `wtf8_step`'s UTF-16 unit count instead of one per sequence, and an astral sequence emits its two halves through `string_from_code_unit` — the one-code-unit constructor `charAt` uses, which encodes a surrogate as WTF-8 and sets HAS_LONE_SURROGATES so `isWellFormed()` and `JSON.stringify` still see a broken half. `limit` counts code units and may cut a pair (`"😀".split("", 1)` is the lone high surrogate). A malformed lead byte reports zero units and still comes back as its own part, keeping the #6085 guarantee that split/join never drops bytes from a Buffer-derived payload. The same walk appears in the two scalar-replacement fast paths — the `split("")[k]` and `split("")[k].length` forms codegen substitutes when the array does not escape — so both were wrong in exactly the same way. They now share one `empty_delimiter_part` locator with the array walk, which is what keeps a scalar-replaced read from disagreeing with the array form of the identical expression. The code-POINT iterators are unchanged and asserted next to the split in the fixture: `[...s]`, `for…of` and the string iterator must keep returning one element per astral character. test-files/test_gap_9409_split_empty_code_units.ts is byte-compared against node. Built from unfixed origin/main the same fixture diverges on 52 lines and throws (`split("")[3]` is undefined for "a<astral>b"). Claude-Session: https://claude.ai/code/session_014knX724SYDogwzsXybCGxp --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
1 participant
ClassUnicodeRange::case_fold_simplewas the second-largest symbol in a symbolized profile ofclaude --helpat 3.78%. This removes essentially all of it: −336,488,547 instructions, −3.78%, wall 0.77 s → 0.74 s, output byte-identical to base and to node.Perry was manufacturing the pathological input itself
JS's "any character" class
[^]was translated to[\s\S], and the empty class[]to[^\s\S].[\s\S]canonicalizes to\x{0}-\x{10FFFF}, andregex_syntax'scase_fold_simplewalks every code point in a range — so under theiflag each such class ran a 1,114,112-step loop to compute a fold that cannot change anything.They now translate to
(?s:.)(Hir::dot, never folded) and[a&&b].(?i)[\s\S]*?(?i)[^\s\S]The real-world driver is five successive
new RegExprefinements of marked's HTML-block rule (iflag, eight[\s\S], and a backreference that forces the fancy engine), plus one URL validator. A probe onget_or_compile_regex— verified able to fire; it saw 3,713 calls — showscc --helpnever matches with any of them. The cost was pure construction.The hypothesis this PR started from was wrong, and was refuted two independent ways
It is worth recording, because the wrong answer was plausible and cheap to believe. The theory was that
std_engine_syntax_ok's\p/\Parm routes through the full HIR translator, so patterns with a Unicode property escape and theiflag pay case folding purely to validate a property name.\parm, and zero of those carryi. The hypothesised combination is empty. Validating all 2,378 costs 9.2 ms total; those 16 are 0.46 ms of it.case_fold_simplecaptured a dwarf stack on every one of its 2,754 calls in a realcc --help— an exhaustive capture, not a sample. 99.57% of calls (4.63% of all retired instructions) go throughjs_regexp_new→compile_and_cache_regex_checked→fancy_regex::Regex::new_options. 0.43% is the legitimate lazy first-use build. Zero pass through the\pvalidation arm.The equivalence question the hypothesis rested on was answered anyway: across all 2,378 literals,
regex_syntax::Parser::parsereturns the same verdict with and without(?i)— 0 disagreements. Sound, and worthless.The actual mechanism was found by asking who calls the hot symbol, exhaustively, rather than by reasoning about which code path looked expensive.
Narrow by construction
Only a six-character class whose members are a shorthand and its complement, outside a character class. Non-capturing group, and
collect_capture_spansindexes the input, so capture numbering is untouched.regressonly ever sees the original JS source, so.sourceis unaffected.Verification
i×\p, bad property names, lookbehind, backreferences, every rewrite form) — run on unchangedmainfirst, where it passed, then base vs node identical, branch vs node identical.perry-runtimesuite;syntax_check_agrees_with_full_buildover 12,077 patterns.An isolated fixture replaying the 59 captured eager constructions goes 574.06M → 257.99M instructions (−55.1%), and symbolized profiles of it put
case_fold_simpleat 57.64% → 10.46% — 304M of the 316M saving is that one symbol.Headroom left, not taken here
The upstream O(range) fold still costs 17.5 ms on the URL validator (
iuflags, large but not full-space ranges); strippingithere drops it to 1.8 ms. Aregex-syntaxfix that walks the fold table instead of the range would recover that — the larger remaining half, and worth an upstream issue rather than another local workaround.Two pre-existing node-parity bugs found in passing, filed separately
The corpus differential also runs against node, and 64 of 3,402 records (1.9%) diverge on
main, unrelated to this change:\w/\W/\buse Rust's Unicode word semantics where JS is ASCII-only, and perry's.excludes only\nwhere JS's also excludes\r/U+2028/U+2029.Summary by CodeRabbit
Performance
Bug Fixes