perf(regex): stride the replace collection loop's safepoint poll - #10666
proggeramlug wants to merge 2 commits into
Conversation
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 |
|
Landed in merge train 221 (#10729), released as v0.5.1599 — main is now Closing rather than merging is how trains work here: the six PRs were cherry-picked onto one tree, validated together, and landed under the train's own commit, so GitHub cannot mark this one merged even though your change is on main. Your commits are in Because close-keywords in a source PR body never fire under this scheme, the issues this resolved were closed from the train's body instead. All 11 across the train are confirmed closed. Validation the tree passed as a whole: 12 gap areas (every one asserted to have run a non-zero number of tests), zero unexplained regressions, artifacts byte-identical to their pin before and after the sweep, both derived integration suites green, and |
1 participant
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