perf(regex): stride the replace collection loop's safepoint poll - #10666
Open
proggeramlug wants to merge 2 commits into
Open
proggeramlug wants to merge 2 commits into
proggeramlug wants to merge 2 commits into
Conversation
added 2 commits
September 18, 2026 19:55
The loop that collects a global replace's matches polled the GC safepoint once per match. That poll costs about 436 instructions -- it evaluates the whole budgeted trigger ladder, which has no cheap "nothing is due" precheck -- and on this loop it enables no collection at all: matches are written into a native span buffer, so the loop creates nothing traced. Measured rather than argued. Nulling this poll entirely moves peak RSS on an allocating replace at n=1,000,000 by +0.0% median over nine interleaved rounds, 4 of 9 rounds in each direction. The same change applied to `Pieces::finish`, which does produce garbage, moved that figure +13.2% with 8 of 9 rounds against -- so this is a controlled contrast between an exposed and an unexposed site, not an assumption that polls are cheap to drop. Instructions, both arms from one commit, release, min of repeated rounds: replace, string template 27,837,069,685 -> 26,852,985,515 -3.5% replace, callback, ASCII 51,289,880,556 -> 50,427,663,998 -1.7% replace, callback, Unicode 61,277,245,689 -> 60,415,684,343 -1.4% replace1m (both, to n=1,000,000) 275,177,993,181 -> 270,666,099,260 -1.6% Peak RSS, nine interleaved rounds on replace1m: median -0.5%, mean +0.4%, 4 of 9 rounds higher. Answers are identical to Node 26.5.1 and to the previous build on every probe. Worst-case work between executed polls does not grow. Every search this loop performs goes through `find_near`, which either polls unconditionally (the owned path and any lent fallback) or ticks `PRE_SEARCH_POLL_TICK` and polls on one search in 64 (PerryTS#10494). That tick advances once per search, which is once per iteration of this loop, so the two strides run in parallel on the same unit rather than composing: the bound stays 64 searches either way. The stride value matches `PRE_SEARCH_POLL_STRIDE` because they must count the same unit, not because 64 is derived. It is a chosen margin in PerryTS#10494 -- the evidence there argues for removing the poll, not for any particular stride -- and nothing here depends on it being the right number, only on not exceeding the value already bounding this path.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe regex replacement collection loop now polls the collector every 64 searches instead of after every search. A constant documents the stride, and the changelog records the instruction reduction and unchanged peak memory. ChangesRegex replacement polling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to The optimization preserves replacement behavior and bounded collector polling, so no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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.
The GC safepoint poll on the replace collection loop, strided 1-in-64. Part of #10165.
This is the second attempt at the poll-frequency lever in this path. The first, #10657, is drafted and will not land: it took −27.9% instructions and +13.2% median peak RSS, over the campaign's ≤ +10% budget. This one is the site where the same lever is free, and the difference is measured rather than assumed.
What it does
The loop that collects a global replace's matches polled the safepoint once per match. That poll costs ~436 instructions — it evaluates the whole budgeted trigger ladder, which has no cheap "nothing is due" precheck — and on this loop it enables no collection at all: matches are written into a native span buffer, so the loop creates nothing traced.
The controlled contrast
Same change, two sites in the same file, nine interleaved rounds on
replace1m(a callback and a template replace at n = 250k/500k/700k/1,000,000 over an 11M-character subject, ~550 MB peak):Pieces::finish(#10657, drafted)4 of 9 is a coin flip: no direction. The predicate that falls out is reusable — polling less is free exactly where the loop produces nothing traced to collect.
Numbers
Both arms from one commit, release, arms verified distinct by md5.
replace1m, both formsPeak RSS on
replace1m, nine interleaved rounds: median −0.5%, mean +0.4%. Answers identical to Node 26.5.1 and to the previous build on every probe, including the correctness differential from #10605 (unset captures, proxy replacers, a replacer allocating hard enough to collect mid-loop, empty matches, non-global, non-string returns).Which axes are measured, and which are asserted
Measured: instructions, peak RSS.
Asserted, with evidence: max time-to-safepoint is not observable today.
gc_runtime_safepoint_pollislet _ = gc_runtime_safepoint_report();and nothing else;perex_runtime::pollreturnsOk(())unconditionally, so it services no interrupt and raises no cancellation. There is no cross-thread safepoint protocol ingc/— nostop_the_world,safepoint_request, rendezvous or handshake — and the source states the collector is "stop-the-world relative to this TLS" (gc/roots/shadow_stack.rs:96,gc/roots/temp_roots.rs:57), i.e. it runs on the allocating thread. The one cross-thread mechanism,GC_UNSAFE_ZONES(gc/policy.rs:4668), is a suppression counter explicitly "without a full stop-the-world mutex", not a rendezvous anyone waits at.And worst-case work between executed polls does not grow. Every search this loop performs goes through
find_near, which either polls unconditionally (the owned path and any lent fallback) or ticksPRE_SEARCH_POLL_TICKand polls on one search in 64 (#10494). That tick advances once per search — once per iteration of this loop — so the two strides run in parallel on the same unit rather than composing: the bound stays 64 searches whether this poll is strided or not.That argument is unit-dependent and the comment says so: striding a site whose counter advanced on a different unit would not be safe on it. Strides that count different units genuinely multiply, which is the concern tracked in #10665.
On the stride value
It matches
PRE_SEARCH_POLL_STRIDEbecause the two must count the same unit — not because 64 is derived. It is a chosen margin in #10494, whose own evidence (removing the poll leftcycle_starts,completionsandstepsidentical across 48,000,000 calls) argues for removal and does not pick a stride. Nothing here depends on 64 being the right number, only on not exceeding the value already bounding this path. If 64 is ever derived properly, both move together and the parallel-unit argument still holds.How stale participation in an incremental cycle can get before it suffers is, as far as I can find, unmeasured anywhere — see #10665.
Validation (local; runners are unreliable)
Base
60922041c.perry-runtimelib suite 4042 passed, 0 failed;--lockedbuild, fmt, regex-off-D warnings, product-D warnings, GC root holders, file size and the release build all OK.Lint gates: 2 of 83 fail, both pre-existing on pristine main and neither reachable from this diff (31 lines in one file, 30 of them comment):
--all-targets; filed as build(codegen): main failscargo check --all-targets— two ImportedClass test initializers miss constructor_has_synthetic_arguments #10655 with the cause and a two-line fix.Thanks to @hello-world-perf's lane, whose questions produced the RSS pairing that drafted #10657, the pointer to measure this site first, and the correction that stopped this PR citing an underived constant as precedent.
Summary by CodeRabbit
Performance
Documentation