perf(regex): build each unique pattern once at construction, not twice#6534
Conversation
js_regexp_new validated an uncached pattern by fully NFA-compiling it (build_std_regex, falling back to build_fancy_regex) and THREW AWAY the result, then get_or_compile_regex re-translated and re-compiled the same pattern to populate the cache. Every unique regex literal in a program paid two Thompson-NFA constructions at startup — a bundle whose modules construct their literals eagerly at load pays this across every literal, and Unicode-class-heavy patterns (the emoji-regex family) cost milliseconds per build. Restructure into compile_and_cache_regex_checked: one translate + one build, seeding REGEX_CACHE (or FANCY_CACHE + the never-match placeholder) and reporting validity; js_regexp_new's validation consumes that, so the get_or_compile_regex that follows is a guaranteed cache hit. get_or_compile_regex keeps its exact fallback behavior (cache + return never-match when both engines reject). One historical edge preserved: validation used to test the BARE translated pattern while the cache built the (?ims)-prefixed one, so a pattern that compiles bare but exceeds the size limit with the prefix was a silent never-match, not a SyntaxError. On checked-compile failure the bare-pattern probe runs as before, keeping that behavior observable unchanged. regex unit tests 42/0.
📝 WalkthroughWalkthroughRegex compilation and caching are centralized in a shared helper with standard-engine and fancy-regex fallback support. Cache eviction is bounded, lookup uses the helper on misses, and ChangesRegex runtime
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RegExp
participant REGEX_CACHE
participant Compiler
participant StandardRegex
participant FancyRegex
RegExp->>Compiler: validate pattern and flags
Compiler->>StandardRegex: compile translated pattern
StandardRegex-->>Compiler: success or failure
Compiler->>FancyRegex: compile when standard engine fails
FancyRegex-->>Compiler: success or failure
Compiler->>REGEX_CACHE: store compiled result
REGEX_CACHE-->>RegExp: cached regex or never-match fallback
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@crates/perry-runtime/src/regex.rs`:
- Around line 280-286: Update the cache handling around REGEX_CACHE and
FANCY_CACHE so evicting a fancy entry also removes its corresponding placeholder
from REGEX_CACHE, preventing stale placeholders from being treated as complete
matches. Preserve standard compiled entries, ensure a subsequent fancy lookup
reconstructs the program and sets fancy_ptr, and add a regression covering
asymmetric cache churn followed by reconstruction of a fancy-only pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 02418202-8a54-4e25-b663-ce4356920b0f
📒 Files selected for processing (1)
crates/perry-runtime/src/regex.rs
| let already = REGEX_CACHE.with(|cache| { | ||
| cache | ||
| .borrow() | ||
| .contains_key(&(pattern.to_string(), flags.to_string())) | ||
| }); | ||
| if already { | ||
| return true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep fancy placeholders and compiled entries under the same eviction lifecycle.
FANCY_CACHE can clear while REGEX_CACHE retains its placeholder. A later lookup treats that placeholder as a complete hit, so a valid fancy-only pattern silently becomes never-match and fancy_ptr remains null.
Clear corresponding placeholders when evicting FANCY_CACHE, or use one cache entry enum holding either the standard or fancy program. Add a regression that reconstructs a fancy pattern after asymmetric cache churn.
Also applies to: 320-343, 348-356
🤖 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-runtime/src/regex.rs` around lines 280 - 286, Update the cache
handling around REGEX_CACHE and FANCY_CACHE so evicting a fancy entry also
removes its corresponding placeholder from REGEX_CACHE, preventing stale
placeholders from being treated as complete matches. Preserve standard compiled
entries, ensure a subsequent fancy lookup reconstructs the program and sets
fancy_ptr, and add a regression covering asymmetric cache churn followed by
reconstruction of a fancy-only pattern.
|
Full-app numbers (197MB compiled TUI bundle, hyperfine 5 runs, isolated HOME) — measured incrementally on top of #6533's link fix, so this is the regex-only delta:
The eliminated work is exactly the discarded validation build of every unique literal at bundle startup. The remaining single eager build is the next lever (lazy/deferred NFA construction), tracked separately. Verification: regex unit tests 42/0; full perry-runtime suite 1327/0 single-threaded; gap shards 5/8 + 7/8 locally green (only reds: pre-triaged + the known local zlib link artifact); 4/4 PTY login e2e runs clean on the rebuilt bundle. |
1 participant
Problem
js_regexp_newvalidates an uncached pattern by fully NFA-compiling it (build_std_regex, falling back tobuild_fancy_regex) — and throws the result away;get_or_compile_regexthen re-translates and re-compiles the same pattern to populate the cache. Every unique regex literal pays two Thompson-NFA constructions at construction time.A bundled app evaluates every module-level literal eagerly at startup, so this doubles regex cost exactly where it hurts: startup profiles of a large compiled TUI app show
regex_automata::nfa::thompson::compiler+regex_syntax::utf8::Utf8Sequencesamong the top frames during the multi-second launch, and Unicode-class-heavy patterns (the emoji-regex family) cost milliseconds per build.Fix
compile_and_cache_regex_checked: one translate + one build, seedingREGEX_CACHE(orFANCY_CACHE+ the never-match placeholder) and reporting validity.js_regexp_new's validation consumes that, making the subsequentget_or_compile_regexa guaranteed cache hit.get_or_compile_regexkeeps its exact fallback (cache + return never-match when both engines reject).One historical edge preserved: validation used to test the BARE translated pattern while the cache built the
(?ims)-prefixed one, so a pattern that compiles bare but exceeds the size limit with the prefix was a silent never-match rather than a SyntaxError. On checked-compile failure the bare-pattern probe still runs, keeping that observable behavior unchanged.Verification
regex unit tests: 42 passed / 0 failed. Full-app startup before/after numbers to follow in a comment once the big-binary rebuild completes.
Summary by CodeRabbit