Merge train 212: record a string template's replacement pieces natively — 1,870 MB to 108 MB (v0.5.1590) - #10492
Merged
Merged
Conversation
added 5 commits
September 17, 2026 12:01
…10411) `str.replace(/re/g, "template")` held roughly a kilobyte of traced heap per output piece until the whole replacement finished. On a 550 KB subject with 100,000 matches that peaked at 545 MB RSS against Node's 122 MB, and it scaled with the number of pieces the template produces rather than with the size of the data: 1,870 MB RSS and 45 s for a 2.2 MB subject. `Pieces` recorded every piece as three `f64` pushed into a JS array — a handle scope and a string addref per push, against an array the collector had to trace and grow. For a string template none of that is needed: every piece is a span of the subject or of the template, both already rooted by the caller and outliving the replacement, and no user code runs between the first match and the last. A callback's pieces still need the list, because the replacement is a string user code produced. `Pieces` gains a native backing used only when a template is present. Records are 12 bytes each, allocated once, charged to the operation's external-byte budget exactly as the span list is, and traced by nobody. `walk` reads them without the per-piece pointer comparisons the list needed to identify each source. The measure-then-copy path, the spec ordering, the span collection and the template parse are unchanged. Measured on perrymaster, subject `"ab12 cd345;".repeat(n)`, 12 passes, release builds from the same base, outputs identical on every row: n=50,000 "[$&]" 545 MB / 5,915 ms -> 74 MB / 846 ms n=50,000 "x" 287 MB / 1,791 ms -> 65 MB / 645 ms n=200,000 "[$&]" 1,870 MB / 45,230 ms -> 108 MB / 3,295 ms callback (control) 161 MB / 1,884 ms -> 164 MB / 1,904 ms Node 26.5.1 on the same rows: 122 MB at n=50,000 and 265 MB / 429 ms at n=200,000 — so the template path now uses less than half of Node's memory, where it used seven times as much.
#[cfg(test)] Cell<usize> that records which backing a replacement used; it holds a count, never a pointer, and is absent from shipped binaries.
…tput
`walk` emits every native record before any list entry, so a `Pieces` holding
both loses the interleaving: on a callback path forced native, each gap lands
before each replacement and the output comes out reordered — a doubled space
where two pieces met and a missing one between records. Silently wrong bytes,
no panic and no error.
The shipped code never mixes them (native is chosen only when a template is
present, and that path never calls `whole`), but the invariant lived in prose,
and the prose was wrong: `new_native`'s comment claimed a mixed caller "still
produces correct output, just without the saving". It does not.
`append` and `whole` now refuse a native backing — a debug assertion naming
the cause, and `EngineError::InvalidSpan` in release — so a mixed caller fails
where the mistake is rather than at `finish`. `walk` asserts the same
invariant. The comment says what is actually true.
Found by perry-b0 running the sabotage direction I had reasoned about rather
than executed: forcing `Pieces::new_native` unconditionally fails on the
callback path's OUTPUT, not on the counter I predicted. Both directions are
now run rather than reasoned:
force Pieces::new -> "a string template must record its pieces
natively", left 0 right 1
force Pieces::new_native -> panics at perex_replace_storage.rs's guard,
"a native Pieces cannot take an arbitrary
source; walk would reorder the output"
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughChangesThe replacement path now stores string-template spans in native Rust records instead of a traced JavaScript piece list. Callback replacements keep heap-backed storage. Tests verify the backing selection. The workspace version and changelog were updated. A near-search safepoint poll was removed as a probe-only change. Native replacement pieces
GC poll experiment
Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant Caller
participant Replace as perex_replace_direct::replace
participant Pieces as perex_replace_storage::Pieces
participant Output
Caller->>Replace: Call replace with string template
Replace->>Pieces: Create native storage
Replace->>Pieces: Append original and template spans
Pieces->>Output: Walk spans and materialize output
Output-->>Caller: Return replacement string
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This train lands #10412 as v0.5.1590, on
7661bc05fe. Four source commits, each verified to preserve its patch-id and authorship.Fixes #10411) —str.replace(/re/g, "template")was holding about a kilobyte of traced heap per output piece.Piecesrecorded every piece as threef64pushed into a JS array: a handle scope and a string addref per push, against an array the collector traced, grew, and retained until the end. For a string template every piece is a span of the subject or of the template, both already rooted, and no user code runs between the first match and the last — so nothing can observe an incremental build. Those records are now 12 native bytes, charged to the same external-byte budget as the span list and traced by nobody."[$&]"At n=50,000: 545 MB / 5,915 ms → 74 MB / 846 ms. So the template path goes from about 7× Node's memory to under half of it, and from 105× its wall to 7.7×. Outputs are identical on every row.
The callback path is untouched by design and stays at RSS parity with Node (161 → 164 MB, 1,884 → 1,904 ms — within noise). A callback's replacement is a string user code produced, so its pieces keep the traced list. This is not the
regex-replace-callbackrows of #10164/#10165, which use a callback and are unaffected.A latent hazard found during review, and fixed here
The first head shipped a comment on
new_nativeclaiming that a mixed caller "still produces correct output, just without the saving". That is false, and the sabotage proved it:walkemits every native record before any list entry, so aPiecesholding both loses the interleaving — every gap lands before every replacement. Forcing native on the callback path produced reordered bytes (a doubled space where two pieces met, a missing one between records) with no panic and no error.The shipped code never mixes them — native is chosen only when a template is present, and that path never calls
whole— but the invariant lived in prose, and the prose was wrong.appendandwholenow refuse a native backing, andwalkasserts the two are never both populated, so a mixed caller fails where the mistake is rather than producing wrong bytes atfinish. The comment now says what is true.Both sabotage directions were run, not reasoned, under
--release:The second is worth stating precisely: the guard has a
debug_assert!and anEngineError::InvalidSpanarm, anddebug_assert!is compiled out of[profile.release]— which is what ships, and which[profile.perry-dev]also inherits. So the releaseErrarm is the load-bearing half; the assertion alone would have enforced the invariant only in a configuration nobody ships. ([profile.gcaudit]exists inCargo.tomlfor the other direction: release codegen withdebug-assertions = true.)The original bug was in #10225. Its acceptance evidence was throughput on short subjects and never measured retention at scale, which is how 1.8 GB of RSS got past review. The general form is worth keeping: a witness has to assert its subject is live on the axis that can regress, not only on the axis it is expected to improve.
Validation
Validated head
e2cbb28e55. Five-package release build pinned and hash-verified, and re-verified after the gap run.main's one known runtime failure. Runtime rises 3984 → 3985, which is the new native-pieces test being registered and actually running.scripts/gc_runtime_root_holders.jsonitself, and the gate passing on the merged tree confirms the pin is right against currentmain, not just against the PR's branch.replace,regex,regexp,string,template,unicode— all six clean, withreplaceandtemplatethe two that directly exercise the changed path. The only red anywhere istest_issue58_object_string, listed verbatim inrun_parity_tests.sh'sSKIP_TESTSonmain.unexplained_regressions={}— no fixture required A/B attribution, which is the first fully clean sweep in this run of trains.Before merging, the pushed head and unchanged main are checked again. After merging, the rewritten commits are checked for preserved authorship and the main tree must match the validated train exactly.
Summary by CodeRabbit
Bug Fixes
String.prototype.replacewith string templates to reduce memory usage and execution time for large inputs.Chores