perf(regex): run the pre-search safepoint poll on one search in 64 (#10166) - #10494
proggeramlug wants to merge 2 commits into
Conversation
|
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 (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe regex runtime adds a strided pre-search safepoint poll for lent-scratch searches. Tests verify two polls across 128 searches. GC runtime-root metadata records the new counter and tick. ChangesPre-search polling
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The bounded polling change has no established current-head correctness or GC-safety defect. It is ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The PR implements a scoped performance improvement for Resolution Run and record the complete Full details: Docstring CoverageExplanation Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (2 skipped: 2 unsupported.)
✨ 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 |
…erryTS#10166) The poll before each search costs 502 of the 4,792 instructions a hoisted `.test()` call takes, and measurement says it buys very little. It cannot cancel. `host::poll` returns `Ok(())` unconditionally, and `EngineError::Cancelled` has no producer anywhere in perry-runtime outside tests, where a test supplies its own cancelling closure to prove the engine's paths clean up. It performs no cycle stepping in practice either. With the poll removed entirely, `cycle_starts`, `completions` and `steps` were IDENTICAL across 48,000,000 allocation-free `.test()` calls interleaved with allocation churn — 5,286 steps on both arms. Every step comes from an allocation-site assist; the What it does retain is the one thing no witness could rule out: the option of servicing a due collection from a loop that allocates nothing, which is how a non-allocating mutator participates in an incremental cycle. Three witness designs failed to construct a program where that mattered, but "could not construct" is not "cannot happen". So the poll is strided rather than removed, keeping a participation point every 64 searches. The stride is a fixed constant, not an environment knob, so it does not owe the GC knob policy an OFF-state CI arm. Measured on perrymaster, both arms from one commit: hoisted .test() 4,792 -> 4,3xx instructions per call regex-replace-callback unchanged at n=700,000, all four GC counters within n=700,000 noise, checksum 203210458 on both arms
b519fec to
9cb4873
Compare
Rebased onto
|
| check | result |
|---|---|
cargo build --locked |
OK |
cargo fmt --all --check |
OK |
cargo test -p perry-runtime --lib -- --test-threads=1 |
3970 passed, 0 failed |
scripts/gc_runtime_root_holders.py (+ --self-test) |
OK — 1465 declarations, 408 classified |
scripts/run_lint_gates.sh |
1 of 83 (public-baseline, the standing CI-only red) |
Ralph's call was no revert: land the stride rather than restoring the unconditional poll first.
|
Landed via merge train #10559 (v0.5.1592). All source commits preserve authorship; merged main matches the validated train exactly. |
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 (#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 #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.
1 participant
Part of #10166. Ralph chose the stride and the value (64, a fixed constant rather than an env knob); the evidence behind that choice is recorded on the issue and summarised here.
What the poll was worth
The
poll()?before each search infind_near_lentcosts 502 of the 4,792 instructions a hoisted.test()call takes. Two things it was assumed to do, it does not:host::pollreturnsOk(())unconditionally, andEngineError::Cancelledhas no producer anywhere inperry-runtimeoutside tests — every construction is a test supplying its own cancelling closure to prove the engine's paths release scratch and preserve consumed work.cycle_starts,completionsandstepswere identical — 5,286 on both arms — across 48,000,000 allocation-free.test()calls interleaved with allocation churn. Every step comes from an allocation-site assist; the perf(gc): make the "nothing due" GC check cheap on safepoint polls and trigger checks #10253 due-check answers "nothing due" and returns.What it does retain is the one thing no witness could rule out: the option of servicing a due collection from a loop that allocates nothing, which is how a non-allocating mutator participates in an incremental cycle. Three witness designs failed to construct a program where that mattered, but "could not construct" is not "cannot happen" — so the poll is strided, not removed, keeping a participation point every 64 searches.
Measurements
perrymaster, both arms from the same base (
e6dcb6274d),perf stat -e instructions:u, string-building control subtracted, outputs identical on every row.re.test(v)re.exec(v)+m[2]438 of the available 502 recovered; the remaining 64 is the tick and branch.
The allocating arm is unchanged.
regex-replace-callbackat n=700,000, whole-program instruction counts because wall clock on this host swung 5,914–9,098 ms under load:cycle_startscompletionsstepsshare_permilleTest
the_pre_search_poll_runs_on_one_search_in_sixty_fourcounts the polls that actually ran over 128 searches and asserts 2 — the path was taken and skipped, not merely "nothing broke".The expected count is pinned to a literal on purpose. My first version derived it as
SEARCHES / PRE_SEARCH_POLL_STRIDE, which made the test self-consistent at any stride: it passed unchanged with the stride set to 1, asserting nothing. That is recorded in the test's own comment so the next person does not reintroduce it.Sabotage-proved after the fix, both directions run rather than reasoned:
PRE_SEARCH_POLL_STRIDE = 1poll_on_stridecallValidation
cargo build --lockedcargo fmt --all --checkcargo check -p perry-runtime --no-default-features --features full,-D warningscargo check -p perry --bins,-D warningscargo test -p perry-runtime --lib -- --test-threads=1scripts/gc_runtime_root_holders.py(+--self-test)scripts/check_file_size.shscripts/run_lint_gates.shThe single failure is
Public benchmark evidence freshness, the long-standing CI-only red. Both new thread-locals carry holder verdicts: the tick isnot_a_gc_pointer(a wrapping count), the test counter istest_only.Scope
The stride is a fixed constant, so it owes the GC knob policy no OFF-state CI arm. Polls between quanta are untouched, so a long single search behaves exactly as before. This does not close #10166 — a hoisted
.test()goes from 10.1× Node to 9.2×, against an acceptance bar of the pre-Perex engine's 1.25×, which is not reachable without the engine's own fixed per-search cost coming down.Summary by CodeRabbit
Performance
Tests