fix(regex): don't recompile a cached pattern just to re-validate it#5777
Conversation
`js_regexp_new` validated every pattern by compiling it with BOTH the `regex` and `fancy-regex` engines (to decide whether to throw `SyntaxError`) — and did so BEFORE consulting `REGEX_CACHE`. So a regex literal evaluated in a hot loop recompiled its automaton on every single call even though `get_or_compile_regex` had already cached the compiled form from the first call. This is pathological for large patterns. string-width's `emojiRegex()` returns a fresh `/…/g` literal (12807 chars, a huge alternation) on every text measurement; ink's flexbox `calculateLayout` measures every cell, so the splash render sat at ~99% CPU for minutes rebuilding the same automaton thousands of times. Fix: skip the expensive validation compile when `(pattern, flags)` is already in `REGEX_CACHE`. A cached pattern is by definition compilable, so the "both engines fail → throw" branch can never fire for it. The cheap JS-syntax checks (`has_invalid_repeated_quantifier`, the `/u`-flag legacy-escape checks) still run on every call, and `get_or_compile_regex` below returns the cached `Arc<Regex>` as before — each `RegExp` object still gets its own fresh header (and thus its own `lastIndex`). Uncached patterns are validated exactly as before. Adds an integration test asserting a repeatedly-compiled literal keeps matching correctly, that two regexes from the same pattern keep independent `lastIndex`, that flags stay part of the cache key, and that invalid patterns still throw on every call. The test runs under a wall-clock timeout so a regression to per-call recompilation fails fast.
📝 WalkthroughWalkthrough
Regex cache validation skip
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry/tests/regexp_cache_repeated_compile.rs (1)
46-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the timeout regression signal.
This timeout only becomes meaningful if the repeated construction is expensive enough to distinguish cache hits from recompilation. With
/(\d+)-(\w+)/over 1000 iterations, the test mostly proves semantic correctness; a recompile regression could still finish well under 30s. Consider swapping in a materially larger pattern (or many more iterations) so the timeout actually exercises the performance regression this PR is fixing.Also applies to: 87-95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/tests/regexp_cache_repeated_compile.rs` around lines 46 - 55, Strengthen the timeout regression test so it actually detects repeated recompilation slowdowns instead of only verifying correctness. Update the repeated-compile benchmark in regexp_cache_repeated_compile.rs, specifically the test that spawns the compiled binary and measures elapsed time, by using a materially larger regex pattern and/or significantly more iterations than the current 1000-loop case. Keep the same timeout-based structure, but make the workload heavy enough that cache hits and recompilation diverge clearly, so the wall-clock timeout meaningfully fails on regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/perry/tests/regexp_cache_repeated_compile.rs`:
- Around line 46-55: Strengthen the timeout regression test so it actually
detects repeated recompilation slowdowns instead of only verifying correctness.
Update the repeated-compile benchmark in regexp_cache_repeated_compile.rs,
specifically the test that spawns the compiled binary and measures elapsed time,
by using a materially larger regex pattern and/or significantly more iterations
than the current 1000-loop case. Keep the same timeout-based structure, but make
the workload heavy enough that cache hits and recompilation diverge clearly, so
the wall-clock timeout meaningfully fails on regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 30487329-e08f-4171-aaac-eb7318b12699
📒 Files selected for processing (2)
crates/perry-runtime/src/regex.rscrates/perry/tests/regexp_cache_repeated_compile.rs
…5808) `js_regexp_new` ran three JS-syntax validation checks (`has_invalid_repeated_quantifier`, `has_unicode_forbidden_legacy_escape`, `has_unicode_forbidden_pattern`) on EVERY call, before the `if !in_cache` gate that already skipped the expensive both-engines recompile (#5777). These checks are not actually cheap: `has_invalid_repeated_quantifier` does a `pattern.chars().collect::<Vec<char>>()` (a ~51 KB allocation for a 12,807-char pattern) plus an O(n) scan. The common `string-width` / `emoji-regex` npm packages construct a fresh ~12,807-char `/…/g` literal on every measurement, and a layout pass can call them thousands of times, so this re-validation — not the already-cached compile — became the top hot frame in profiles. Move all three checks inside the existing `if !in_cache { ... }` block (with `in_cache` computed first), so the whole validation block is skipped on a REGEX_CACHE hit. This is safe: regex validity is a pure function of (pattern, flags); an invalid pattern throws before `get_or_compile_regex` can cache it; and both writers of REGEX_CACHE (`js_regexp_new` and `regex/compile.rs`) run these exact checks before populating the cache, so any cache hit is provably valid. Invalid patterns are never cached, so they remain cache misses and still throw SyntaxError on every call. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
Problem
js_regexp_newvalidates every pattern by compiling it with both theregexandfancy-regexengines (to decide whether to throwSyntaxError) — and does so before consultingREGEX_CACHE. So a regex literal evaluated in a hot loop recompiled its automaton on every single call, even thoughget_or_compile_regexhad already cached the compiled form on the first call.This is pathological for large patterns. string-width's
emojiRegex()returns a fresh/…/gliteral (12807 chars — a huge alternation) on every text measurement, and ink's flexboxcalculateLayoutmeasures every cell. The result: rendering the splash sat at ~99% CPU for minutes, rebuilding the same automaton thousands of times.Backtrace of the spin:
Fix
Skip the expensive validation compile when
(pattern, flags)is already inREGEX_CACHE. A cached pattern is by definition compilable, so the "both engines fail → throw" branch can never fire for it.has_invalid_repeated_quantifier, the/u-flag legacy-escape checks) still run on every call.get_or_compile_regexstill returns the cachedArc<Regex>, and eachRegExpobject still gets its own fresh header (its ownlastIndex).Net effect: 30,000 fresh-literal compiles of a large pattern drop from minutes to ~25ms (cache hit).
Test
regexp_cache_repeated_compile.rsasserts a repeatedly-compiled literal keeps matching correctly, two regexes from the same pattern keep independentlastIndex, flags stay part of the cache key, and invalid patterns still throw on every call — run under a wall-clock timeout so a regression to per-call recompilation fails fast.Summary by CodeRabbit
Performance Improvements
Bug Fixes
lastIndexbehavior independent for separate regex instances created from the same pattern.