fix(gc): warm_generator_intrinsics must call the tower builder, not a no-op - #7731
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe generator test warm-up now calls ChangesGenerator intrinsic initialization
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
… no-op crates/perry-runtime/src/gc/tests/runtime_roots/generator_attach_prototype.rs's three tests were red on main: js_generator_attach_prototype and js_generator_attach_closure_prototype no longer moved their receiver under an alloc-point copying minor, and the shipped-default witness never saw its trigger armed. warm_generator_intrinsics() called js_generator_attach_prototype(TAG_UNDEFINED, 0) to pre-build the generator intrinsic tower before the timed call under test. That never worked: js_generator_attach_prototype returns at its very first line for any non-pointer obj, so the "warm-up" touched nothing. It went unnoticed because GENERATOR_FUNCTION_INTRINSIC_PTR and its five siblings were plain process-global AtomicI64s pre-#7723 - some earlier test in the same binary had almost always already built the tower, so the real call under test found it cached regardless of what warm_generator_intrinsics() did. #7723 converted those six statics to per_test_global! specifically so each test starts from a guaranteed first-touch state (crates/perry-runtime/src/gc/tests/lazy_intrinsic_towers.rs's whole point). That is a correct, deliberate change - it took away the accidental cross-test priming these three tests had been relying on. With nothing pre-built, the real call now pays the dozens-of-allocations tower build itself, inside build_generator_tower's GcSuppressScope (#7251's no-move window for that build). That suppression window swallows the arena trigger the test injected via arm_collection_on_next_block for the rest of the call: no copying minor ever runs before the tower build's own scope closes, and by then intermediate's own allocation no longer needs a new arena block, so the trigger is never serviced. Confirmed with instrumented gc_check_trigger / GcSuppressScope traces comparing the last-good commit against #7723: on the last-good commit the real call's first allocation reaches gc_check_trigger unsuppressed and services the trigger directly; on #7723 the entire ~1800-call tower build runs suppressed first and nothing ever re-triggers afterward. Fix warm_generator_intrinsics() to call crate::object::ensure_generator_intrinsics() directly - the same builder lazy_intrinsic_towers.rs uses - so it does what its name and doc comment always claimed. This does not touch the liveness/deferral assertions those tests make; it only repairs the test's own setup helper. Bisected via git checkout of each of today's three merges in an isolated worktree: c907953 (pre-#7721) passes; ca8c0d6 (#7721, moving-loop poll default flip) passes; cbb682d (#7723, no-move window + per_test_global towers) is the first commit where all three fail. #7724 is uninvolved.
7687752 to
5a7a852
Compare
Merging as v0.5.1422 — this unblocks
|
| commit | result |
|---|---|
c9079531d (pre-#7721) |
3/3 pass |
ca8c0d617 (#7721, poll default flip) |
3/3 pass |
cbb682d31 (#7723) |
3/3 fail — first bad commit |
c156f8a41 (#7724) |
uninvolved |
The diagnosis is the interesting part, and it is not "#7723 broke it"
warm_generator_intrinsics() called js_generator_attach_prototype(TAG_UNDEFINED, 0), expecting it to build the generator tower as a side effect. It never did — that function returns at its very first line for any non-pointer obj. The warm-up was dead code from the day it was written.
It went unnoticed because the six tower AtomicI64 statics were process-global: some earlier test in the same binary had almost always already built the tower, so the real call under test found it cached no matter what the warm-up did. The test passed for a reason that had nothing to do with its setup.
#7723 converted those statics to per_test_global! precisely so every test starts from a guaranteed first-touch state — which is its whole point for lazy_intrinsic_towers. That removed the accidental cross-test priming and exposed the dead helper. The real call then paid the ~1800-call tower build itself, inside build_generator_tower's GcSuppressScope, which swallowed the injected arena trigger; by the time suppression lifted, no allocation needed a new block, so the trigger was never serviced and GC_SAFEPOINT_PENDING stayed false.
So both of #7723's changes are correct. This is a latent bug in the test's own setup, unmasked by an isolation fix — which is exactly what test isolation is for. Fixed by calling crate::object::ensure_generator_intrinsics() directly, the same builder lazy_intrinsic_towers.rs uses.
The liveness assertions are untouched, and they are why any of this was found: "LIVE SUBJECT: the trigger must actually have been due and deferred. Without this the test also passes when nothing was armed at all." Without that guard these three would have gone green on a build where nothing was armed, and the dead warm-up would still be there.
Verification
cargo test -p perry-runtime --lib --no-fail-fast: 1961 passed, 0 failed, 4 ignored. The three target tests pass 3/3 in isolation with --test-threads=1.
On my side
I merged #7721/#7723/#7724 without running cargo test -p perry-runtime --lib — my local gate covered the lint job's 19 steps but not the cargo-test job at all, so it reported green while a gated check was red. That is the same defect as a partial gate reporting on commands it never ran, one level up. The gate now runs perry-runtime --lib and perry-codegen --lib too, and it caught this immediately on the next PR.
Summary
cargo test -p perry-runtime --libwas red onmain(blocking every open PRvia the per-PR
--lib --binsgate), with all threegc::tests::runtime_roots::generator_attach_prototypetests failing:attach_prototype_survives_an_alloc_point_copying_minor_inside_the_callattach_closure_prototype_survives_an_alloc_point_copying_minor_inside_the_callthe_shipped_default_defers_the_trigger_out_of_the_callees_windowRoot cause
Bisected in an isolated worktree by checking out each of today's three
merges (
c9079531d→ca8c0d617(#7721) →cbb682d31(#7723) →c156f8a41(#7724)) and running just this test file:c9079531d(pre-fix(gc): make the moving-loop poll default ON in the code, not just the doc (#7690, #7682) #7721): all 3 passca8c0d617(fix(gc): make the moving-loop poll default ON in the code, not just the doc (#7690, #7682) #7721, moving-loop poll default flip): all 3 passcbb682d31(gc: no-move window for lazy intrinsic towers (#7251) + CI arm for ZEAL+VERIFY_EVACUATION (#7254) #7723, no-move window for lazy intrinsic towers +per_test_global!conversion): all 3 fail — first bad commitc156f8a41(gc: fix root-dominance phi false positives + 3 of 5 real hits, lower --max-unrooted to 2 (#7664) #7724, root-dominance phi fixes): uninvolved, already broken#7723 did not break
js_generator_attach_prototype/js_generator_attach_closure_prototype. It correctly converted the sixgenerator-intrinsic-tower
AtomicI64statics(
GENERATOR_FUNCTION_INTRINSIC_PTRand siblings) from plain process-globalsto
per_test_global!, so every test starts from a guaranteed first-touch"tower not yet built" state instead of whichever test happened to run first
in the process silently pre-warming it for everyone else. That is exactly
what
gc::tests::lazy_intrinsic_towersneeds to gate #7251, and it is theright fix for a real cross-test hazard.
What it exposed was a latent bug in this test file's own setup helper:
js_generator_attach_prototypereturns at its very first line for anynon-pointer
obj(!jv.is_pointer()), so this call never reachedgenerator_prototype_ptr/ensure_generator_intrinsics()— it always was ano-op. Pre-#7723 that didn't matter: the tower statics were process-global,
and by the time this test ran, some earlier test in the same binary had
almost always already built the tower, so the real call under test found it
cached regardless of what
warm_generator_intrinsics()did.Once the towers became per-thread (per-test), that accidental priming went
away. With nothing pre-built, the real call now pays the
dozens-of-allocations tower build itself, inside
build_generator_tower'sGcSuppressScope(#7251's own no-move window for that build — also correct).Traced with instrumented
gc_check_trigger/GcSuppressScopelogging: onthe last-good commit the real call's first allocation reaches
gc_check_triggerunsuppressed and services the test's injected triggerdirectly (1 relevant call); on #7723 the ~1800-call tower build runs entirely
inside one suppression window first, swallowing every trigger check that
happens during it, and by the time the window closes the following
allocations no longer need a new arena block, so the trigger is never
serviced — "subject not live" in the two alloc-point tests, and the
deferral witness's
GC_SAFEPOINT_PENDINGnever gets set in the third.Judgement call
This is neither "the PR broke behaviour" nor "the test's premise is now
wrong" — #7723's production and test-isolation changes are both correct and
their invariants are unaffected. It's a pre-existing bug in this test file's
own helper, masked by accident until the isolation fix removed the accident.
Fixed the helper to call the real builder
(
crate::object::ensure_generator_intrinsics(), the same onelazy_intrinsic_towers.rsuses) instead of a call shape that never reachedit. The liveness/deferral assertions in the three tests are untouched.
Test plan
cargo test -p perry-runtime --lib generator_attach_prototype -- --test-threads=1— 3/3 pass, repeated 3x for flake-checkingcargo test -p perry-runtime --lib lazy_intrinsic_towers -- --test-threads=1— 2/2 pass (adjacent gc: no-move window for lazy intrinsic towers (#7251) + CI arm for ZEAL+VERIFY_EVACUATION (#7254) #7723 gate, unaffected)cargo test -p perry-runtime --lib --no-fail-fast -- --test-threads=1— 1961 passed, 0 failed, 4 ignored (one unrelated timing-sensitive test,
promise::keyed_table::tests::settling_many_keys_is_not_quadratic, flakedonce under host load 40–70 and passed clean on every isolated re-run;
confirmed unrelated to this change)
cargo fmt --all -- --checkcleanSummary by CodeRabbit