test(gc): dependency-scale runtime witness for the moving collector (#7717) - #7793
Conversation
0bfcbb9 to
2bcf80c
Compare
|
Warning Review limit reached
Next review available in: 17 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a dependency-scale moving-GC runtime witness. The witness compiles and runs a zod workload, compares baseline and GC-zeal output, validates moving-GC diagnostics, and runs in CI after dependency installation. ChangesDependency-scale GC validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant gc_dep_scale_witness_sh
participant PerryCompiler
participant MovingGCRuntime
CI->>gc_dep_scale_witness_sh: install dependencies and run witness
gc_dep_scale_witness_sh->>PerryCompiler: compile zod workload
gc_dep_scale_witness_sh->>MovingGCRuntime: run baseline workload
gc_dep_scale_witness_sh->>MovingGCRuntime: run GC-zeal workload with quarantine
MovingGCRuntime-->>gc_dep_scale_witness_sh: return output and diagnostics
gc_dep_scale_witness_sh-->>CI: pass or fail witness assertions
Possibly related issues
Possibly related PRs
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/gc-moving-witnesses.yml:
- Around line 380-382: The relevance filter used by the dependency-scale runtime
witness must include changes to scripts/gc_dep_scale_witness.sh,
test-files/gc-dep-corpus/, package.json, and package-lock.json. Update the
collector-relevant expression near the existing relevance configuration so
changes to any of these inputs cause steps.relevance.outputs.run to be true and
execute the witness.
In `@scripts/gc_dep_scale_witness.sh`:
- Around line 166-168: Update the retired-count calculation in
scripts/gc_dep_scale_witness.sh to match only from-space-protect diagnostics
that explicitly report a page-set retirement, rather than every
“[gc-fromspace-protect]” line. Keep the existing zero-count fallback and failure
check so the script passes only when a retirement diagnostic is observed.
- Around line 85-88: Update the validation flow in
scripts/gc_dep_scale_witness.sh after the existing zod and ENTRY checks to count
emitted native modules and enforce the MIN_MODULES threshold, reusing the check
and counting approach from gc_root_dominance_dep_corpus.sh. Ensure thin or
non-native corpora fail through the script’s existing fail mechanism.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ed8b595-6153-454c-bf4a-565f2c87379a
📒 Files selected for processing (3)
.github/workflows/gc-moving-witnesses.ymlchangelog.d/7793-dep-scale-runtime-witness.mdscripts/gc_dep_scale_witness.sh
| - name: Run the dependency-scale runtime witness | ||
| if: steps.relevance.outputs.run == 'true' | ||
| run: ./scripts/gc_dep_scale_witness.sh |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Run this job when dependency-scale witness inputs change.
Line 381 depends on the relevance filter. That filter does not include scripts/gc_dep_scale_witness.sh, test-files/gc-dep-corpus/, package.json, or package-lock.json. A pull request that changes only these inputs skips this witness.
Add these paths to the collector-relevant expression at line 189.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/gc-moving-witnesses.yml around lines 380 - 382, The
relevance filter used by the dependency-scale runtime witness must include
changes to scripts/gc_dep_scale_witness.sh, test-files/gc-dep-corpus/,
package.json, and package-lock.json. Update the collector-relevant expression
near the existing relevance configuration so changes to any of these inputs
cause steps.relevance.outputs.run to be true and execute the witness.
| # The dependency IS the test. Its absence must be an error rather than a | ||
| # silently smaller witness. | ||
| [ -f node_modules/zod/src/index.ts ] || fail "node_modules/zod/src/index.ts is missing — run: npm ci --ignore-scripts --no-audit --no-fund" | ||
| [ -f "$ENTRY" ] || fail "$ENTRY is missing" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '1,180p' scripts/gc_root_dominance_dep_corpus.sh
sed -n '70,120p' scripts/gc_dep_scale_witness.sh
rg -n -C 3 'MIN_MODULES|module' \
scripts/gc_root_dominance_dep_corpus.sh \
scripts/gc_dep_scale_witness.shRepository: PerryTS/perry
Length of output: 17099
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- gc_dep_scale_witness.sh ---'
nl -ba scripts/gc_dep_scale_witness.sh | sed -n '1,230p'
printf '%s\n' '--- dependency corpus entry and imports ---'
nl -ba test-files/gc-dep-corpus/main.ts | sed -n '1,180p'
rg -n -C 3 'node_modules/zod|gc-dep-corpus|PERRY_TRACE|trace|module|MIN_MODULES' \
scripts/gc_dep_scale_witness.sh \
test-files/gc-dep-corpus \
package.json package-lock.jsonRepository: PerryTS/perry
Length of output: 223
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- gc_dep_scale_witness.sh ---'
awk '{printf "%6d %s\n", NR, $0}' scripts/gc_dep_scale_witness.sh | sed -n '1,230p'
printf '%s\n' '--- dependency corpus entry and imports ---'
awk '{printf "%6d %s\n", NR, $0}' test-files/gc-dep-corpus/main.ts | sed -n '1,180p'
rg -n -C 3 'node_modules/zod|gc-dep-corpus|PERRY_TRACE|trace|module|MIN_MODULES' \
scripts/gc_dep_scale_witness.sh \
test-files/gc-dep-corpus \
package.json package-lock.jsonRepository: PerryTS/perry
Length of output: 34555
Add the MIN_MODULES check
This script only checks that zod and the entry file exist. It does not count emitted native modules. Reuse the MIN_MODULES check from scripts/gc_root_dominance_dep_corpus.sh so a thin or non-native dependency corpus cannot pass.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/gc_dep_scale_witness.sh` around lines 85 - 88, Update the validation
flow in scripts/gc_dep_scale_witness.sh after the existing zod and ENTRY checks
to count emitted native modules and enforce the MIN_MODULES threshold, reusing
the check and counting approach from gc_root_dominance_dep_corpus.sh. Ensure
thin or non-native corpora fail through the script’s existing fail mechanism.
| retired="$(grep -cF '[gc-fromspace-protect]' "$OUT_DIR/zeal.err" || true)" | ||
| : "${retired:=0}" | ||
| [ "$retired" -gt 0 ] || fail "the from-space quarantine never retired a page-set, so PERRY_GC_PROTECT_FROMSPACE=1 protected NOTHING and this run's cleanliness means nothing (#7717)." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match retired page-set diagnostics only.
Line 166 counts every [gc-fromspace-protect] diagnostic. A non-retirement diagnostic can make retired positive. The script can then pass without proving that a page-set was retired.
Proposed fix
-retired="$(grep -cF '[gc-fromspace-protect]' "$OUT_DIR/zeal.err" || true)"
+retired="$(grep -cF '[gc-fromspace-protect] retired_set=' "$OUT_DIR/zeal.err" || true)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| retired="$(grep -cF '[gc-fromspace-protect]' "$OUT_DIR/zeal.err" || true)" | |
| : "${retired:=0}" | |
| [ "$retired" -gt 0 ] || fail "the from-space quarantine never retired a page-set, so PERRY_GC_PROTECT_FROMSPACE=1 protected NOTHING and this run's cleanliness means nothing (#7717)." | |
| retired="$(grep -cF '[gc-fromspace-protect] retired_set=' "$OUT_DIR/zeal.err" || true)" | |
| : "${retired:=0}" | |
| [ "$retired" -gt 0 ] || fail "the from-space quarantine never retired a page-set, so PERRY_GC_PROTECT_FROMSPACE=1 protected NOTHING and this run's cleanliness means nothing (`#7717`)." |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/gc_dep_scale_witness.sh` around lines 166 - 168, Update the
retired-count calculation in scripts/gc_dep_scale_witness.sh to match only
from-space-protect diagnostics that explicitly report a page-set retirement,
rather than every “[gc-fromspace-protect]” line. Keep the existing zero-count
fallback and failure check so the script passes only when a retirement
diagnostic is observed.
2bcf80c to
adbd197
Compare
2e8a4da to
c7fe3e0
Compare
c7fe3e0 to
647b972
Compare
|
Audit complete — merging, with one substantive change and one finding. The change: ported from Re-measured post-port (release build of this tree = main's runtime): PASS, 8,022 forced collections, all copying minors, 852,411 objects relocated, 63,936 back-edge polls, 8,022 retired page-sets, answer byte-identical with and without the relocating collector — within noise of the PR's pre-port zeal numbers (8,020 / 852,107 / 63,936), which is itself a nice confirmation that rate-1 IS the retired instrument. Sabotage state after the port:
Also folded in: the workflow comment and fragment updated to the schedule vocabulary, with the port recorded in the fragment. The gate stays out of required contexts per the PR's own reasoning — observe on Closes #7717. |
1 participant
The moving collector now has a dependency-scale RUNTIME witness in CI (#7717, the one unmet ask of #7280).
Everything gating this family was either a synthetic fixture or a static pass over emitted IR, and neither can observe what the #7154 class actually presents as.
scripts/gc_root_dominance_check.pyreads LLVM IR, so a runtime-sidestaticorthread_local!holding a raw heap pointer is structurally invisible to it; and the defect is invisible to every runtime GC probe at the collection too, because there is nothing for the collector to find — it surfaces cycles later, in a different function, asTypeError: value is not a function.Scale is the point, not size. #7280 measured it: the curated 25-file corpus passed 25/25 while twenty lines of stock
zodfailed 5/40, because dependency-shaped code is dominated byjs_object_assign_one(object spread) andjs_new_function_construct— populations the curated files barely produce.scripts/gc_dep_scale_witness.shrunstest-files/gc-dep-corpus/main.ts— the same 81-module zod corpusgc-root-dominancealready compiles for its static check, whose own header always said it "is run as the acceptance workload for the moving collector" — underPERRY_GC_ZEAL=1 PERRY_GC_PROTECT_FROMSPACE=1 PERRY_GC_PROTECT_FROMSPACE_DEPTH=800, and is wired intogc-moving-witnesses.yml.Measured on
main: 8,020 forced collections, all 8,020 of them copying minors, 852,107 objects relocated, 63,936 back-edge polls, 8,020 retired quarantine page-sets, and the answer byte-identical to the same binary run without zeal.What makes it able to fail. The trap #7717 names is a run that exits clean having witnessed nothing, so the script asserts its subject ran three independent ways: the
[gc-zeal]exit verdict;copying_minors/moved_objects/loop_pollsre-read from it rather than trusting that the binary still exits 70; and a non-zero count of retired from-space page-sets, becausePERRY_GC_PROTECT_FROMSPACE=1on a run with no copying minor protects nothing and still exits 0. Both directions were verified by sabotage rather than asserted: with zeal disabled it fails on the missing verdict, and with the quarantine disabled it fails with "protected NOTHING and this run's cleanliness means nothing" — which is precisely the wrong conclusion #7717 records nearly recording.Two findings worth keeping:
PERRY_NO_AUTO_OPTIMIZE=1is load-bearing here, and is a deliberate departure fromgc_repsel_matrix.sh, which omits it on purpose. Without it the compile reaches the auto-optimizer, which relinks the runtime asfeatures=async-runtime,web-fetch— nodiagnostics, anddiagnosticsis what emits the[gc-fromspace-protect]line this gate reads as its proof. The assertions are fail-closed so a stripped runtime goes red rather than quietly green, but red-for-the-wrong-reason is still a broken gate.sets_held=800/800), so lowering it silently narrows the window in which a stale dereference is still catchable. The default of 4 is far too small for real code — GC: evacuating minor drops an old-to-young field[1] edge, crashing with 'value is not a function' #7154's own reproducer needed 800.The gate is deliberately not added to branch protection's required contexts: a new gate has never been green, so promoting it immediately would block every open PR. It should be observed on
mainfirst, then promoted — leaving that second step undone is howgc-stressended up reporting failures without blocking anything.Also corrected while writing this: #7717 says loop polls are "default off since #7161". They are default ON since #7721 —
moving_safepoint_polls_enabled_from_envis!matches!(value, Some("0") | Some("off") | Some("false"))— so the witness does not need to opt into them, and the run above confirms 63,936 polls fired without the flag.Validation
./scripts/gc_dep_scale_witness.shonmain@0a2bf15bd, release build: PASS, exit 0.PERRY_GC_ZEAL=0: exit 1,::error::no [gc-zeal] verdict line was printed.PERRY_GC_PROTECT_FROMSPACE=0: exit 1,::error::the from-space quarantine never retired a page-set.bash -nclean, workflow parses underyaml.safe_load,scripts/check_file_size.shexit 0, nocontinue-on-erroron any step of the job.Summary by CodeRabbit
Tests
Documentation