ci(compiler-output-regression): restore the native-region-proof gate that is red on main - #7158
Merged
Merged
Conversation
…shading barrier The native-region-proof gate went red on main after PerryTS#7088 moved the shadow-slot root store — and its incremental-mark root-shading barrier — from a js_shadow_slot_bind/js_shadow_slot_set runtime call to inline IR. The barrier was always emitted; it lived inside the runtime function and was invisible to the harness's static call counter. Inlining exposed the js_write_barrier_root_nanbox call site, so write_barriers_static jumped (h1_native_rep_equivalence 0->3, one per rooted Buffer local) and every affected workload tripped its heap-barrier budget. The same lowering adds ss.* blocks ahead of the module-init loops, shifting the deterministic per-function block counter by 12 and blanking the region labels (for.body.2/6/10 -> 14/18/22). Not a real regression: the barrier is gated behind PERRY_INCREMENTAL_MARK_BARRIER_ACTIVE_COUNT, never fires here (write_barriers_traced stays 0), sits in guarded ss.barrier blocks at root-bind sites (never inside the native loops, which keep raw load/store i8 + alias metadata and no runtime calls), and PerryTS#7088 proves it observationally identical to the call it replaced. - structural_counters scores write_barriers_static on the optimizer-controlled heap barriers (js_write_barrier, js_write_barrier_slot) only; the shadow-stack root-shading barriers (js_write_barrier_root_nanbox, js_write_barrier_root_heap_word) move to a new, reported-but-non-gating root_shading_barriers_static field. Real regressions stay caught: heap barriers are still counted, and a root barrier that actually fires is caught by the write_barriers_traced budget. - h1_native_rep_equivalence region selectors follow the renumbered loop bodies. Bisected to 91f1e7c (PerryTS#7088). Verified: harness unit tests green; h1_native_rep_equivalence + scalar_replacement_literals gate green; the full suite now matches the PerryTS#7088 parent (CI-green) locally, the only residual failures being pre-existing macOS/clang-16 env noise (smax.i32, clang loop safepoint placement) identical on both.
📝 WalkthroughWalkthroughThe compiler-output harness now reports inline root-shading barriers separately from heap write barriers. Regression expectations and H1 region selectors were updated for the resulting barrier counts and LLVM block labels. ChangesNative region proof updates
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@changelog.d/7136-native-region-proof-inline-root-barrier.md`:
- Around line 2-4: Update the prose in the changelog entry to avoid leading
hash-prefixed issue references being parsed as Markdown headings: replace the
visible “#7088” references with “Issue 7088” while preserving the existing
wording.
In `@scripts/compiler_output_harness/analyzers.py`:
- Around line 224-240: Add focused runtime coverage for both
runtime_write_barrier_root_nanbox() and runtime_write_barrier_root_heap_word()
in write_barriers_traced, verifying each emits write_barrier.calls when
incremental marking is active and does not emit it when inactive. Ensure the
tests cover the fallback path so structural_counters cannot permit removed
root-shading barriers.
🪄 Autofix (Beta)
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: 504c4bc6-14e9-4009-8e08-f73f5457faa3
📒 Files selected for processing (4)
benchmarks/compiler_output/workloads.tomlchangelog.d/7136-native-region-proof-inline-root-barrier.mdscripts/compiler_output_harness/analyzers.pytests/test_compiler_output_regression.py
Comment on lines
+2
to
+4
| #7088's inline shadow-slot root barrier, restoring green on `main`. | ||
|
|
||
| #7088 moved the per-store shadow-stack root store — and its |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Avoid Markdown heading parsing for issue references.
The leading #7088 tokens on Line 2 and Line 4 trigger markdownlint MD018. Write Issue 7088 or escape the hash so these lines remain prose.
Proposed Markdown fix
-#7088's inline shadow-slot root barrier, restoring green on `main`.
+Issue 7088's inline shadow-slot root barrier, restoring green on `main`.
-#7088 moved the per-store shadow-stack root store — and its
+Issue 7088 moved the per-store shadow-stack root store — and its📝 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.
Suggested change
| #7088's inline shadow-slot root barrier, restoring green on `main`. | |
| #7088 moved the per-store shadow-stack root store — and its | |
| Issue 7088's inline shadow-slot root barrier, restoring green on `main`. | |
| Issue 7088 moved the per-store shadow-stack root store — and its |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 2-2: No space after hash on atx style heading
(MD018, no-missing-space-atx)
[warning] 4-4: No space after hash on atx style heading
(MD018, no-missing-space-atx)
🤖 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 `@changelog.d/7136-native-region-proof-inline-root-barrier.md` around lines 2 -
4, Update the prose in the changelog entry to avoid leading hash-prefixed issue
references being parsed as Markdown headings: replace the visible “#7088”
references with “Issue 7088” while preserving the existing wording.
Source: Linters/SAST tools
Comment on lines
+224
to
+240
| # Heap write barriers — the perf-relevant, optimizer-controlled | ||
| # barriers this gate exists to catch. Counted statically because a | ||
| # native region that stores a GC pointer into a heap object needs | ||
| # one; the proof budgets bound how many. | ||
| "write_barriers": after_calls.get("js_write_barrier", 0) | ||
| + after_calls.get("js_write_barrier_slot", 0) | ||
| + after_calls.get("js_write_barrier_root_nanbox", 0) | ||
| + after_calls.get("js_write_barrier_slot", 0), | ||
| # GC shadow-stack root-shading barriers (#7088). Before #7088 these | ||
| # lived inside the `js_shadow_slot_bind` / `js_shadow_slot_set` | ||
| # runtime calls and were invisible to this static IR counter; #7088 | ||
| # emits the shadow-slot store — and its barrier — inline, so the | ||
| # call site is now visible here. Each is emitted once per rooted | ||
| # pointer-capable local (a structural constant, not an optimizer | ||
| # choice) and is gated behind PERRY_INCREMENTAL_MARK_BARRIER_ACTIVE_COUNT, | ||
| # so it never fires unless incremental marking is live — meaning it | ||
| # is caught, if it ever regresses at runtime, by write_barriers_traced. | ||
| # Tracked separately so it does not inflate the heap-barrier budget. | ||
| "root_shading_barriers": after_calls.get("js_write_barrier_root_nanbox", 0) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'write_barrier_calls|js_write_barrier_root_(nanbox|heap_word)|PERRY_INCREMENTAL_MARK_BARRIER_ACTIVE_COUNT' .Repository: PerryTS/perry
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== runtime implementations containing root barrier calls =="
rg -n -C 8 \
'js_write_barrier_root_(nanbox|heap_word)|runtime_write_barrier|write_barrier_calls|trace' \
crates/perry-runtime crates/perry-core scripts || true
echo
echo "== analyzers relevant functions =="
sed -n '420,510p' scripts/compiler_output_harness/analyzers.py
sed -n '140,255p' scripts/compiler_output_harness/analyzers.py
echo
echo "== test regression relevant section =="
sed -n '1500,1590p' tests/test_compiler_output_regression.pyRepository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files defining root barrier implementations =="
rg -n -C 12 \
'fn js_write_barrier_root_(nanbox|heap_word)|pub.*js_write_barrier_root_(nanbox|heap_word)|js_write_barrier_root_(nanbox|heap_word)\(' \
crates/perry-runtime/src 2>/dev/null || true
echo
echo "== runtime_write_barrier tracing helpers =="
rg -n -C 10 \
'runtime_write_barrier(_root_(nanbox|heap_word))?|record_write_barrier|gc_cycle_trace|write_barrier_calls|take_write_barrier_trace' \
crates/perry-runtime/src/gc crates/perry-runtime/src 2>/dev/null \
--glob '!**/target/**' \
--glob '!**/tests/**' || true
echo
echo "== analyzer summary and structural_counter gate =="
sed -n '200,245p' scripts/compiler_output_harness/analyzers.py
sed -n '430,510p' scripts/compiler_output_harness/analyzers.py
echo
echo "== tests using root-barrier runtime counters =="
rg -n -C 8 \
'write_barriers_traced|write_barrier_calls|js_write_barrier_root_(nanbox|heap_word)|root_shading_barriers_static' \
tests scripts crates/perry-runtime/src/gc/tests 2>/dev/null || trueRepository: PerryTS/perry
Length of output: 50370
Add runtime coverage for root-shading barriers.
runtime_write_barrier_root_nanbox() and runtime_write_barrier_root_heap_word() return without emitting write_barrier.calls when the cycle is inactive. Since these helpers are removed from write_barriers_static, fallbacks like structural_counters can allow removed root barriers unless write_barriers_traced has a focused test for both root helpers.
🤖 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/compiler_output_harness/analyzers.py` around lines 224 - 240, Add
focused runtime coverage for both runtime_write_barrier_root_nanbox() and
runtime_write_barrier_root_heap_word() in write_barriers_traced, verifying each
emits write_barrier.calls when incremental marking is active and does not emit
it when inactive. Ensure the tests cover the fallback path so
structural_counters cannot permit removed root-shading barriers.
jdalton
changed the title
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
2 participants
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
compiler-output-regressionmerge gate is failing on a clean checkout oforigin/main. Because it is a required check, that failure blocks every open pull request in the repository, whatever the pull request actually changes. Nothing is wrong with the compiler. The gate is counting something it was never meant to count, because a recent change moved that thing somewhere the gate could see it.Some background on what the gate measures. Its
native-region-proofsuite proves two properties of the generated code: that hot native loops contain no runtime calls, and that the compiler emits no more write barriers than a per-workload budget allows. A write barrier is a short piece of code the compiler inserts next to a pointer store so the garbage collector learns that a new reference exists. The suite counts barriers by scanning the generated intermediate representation for barrier call sites, so it is sensitive to where a barrier physically sits, not to whether that barrier ever runs.This change splits that one count into two.
write_barriers_staticnow counts only the heap write barriers the optimizer controls (js_write_barrierandjs_write_barrier_slot), which are the barriers this gate exists to police. The garbage collector's shadow-stack root-shading barriers (js_write_barrier_root_nanboxandjs_write_barrier_root_heap_word) move to a newroot_shading_barriers_staticfield, which is reported but does not gate. Theh1_native_rep_equivalenceworkload's region selectors are also pointed at the loop-body labels the compiler emits today. No compiler code changes here, only the Python harness and its TOML fixtures.Bisecting the red gate to one commit
Running
git bisectwithh1_native_rep_equivalenceas the probe pins the flip to91f1e7c82,perf(gc): emit the shadow-slot root store inline (#7088), authored by Ralph Küpper. The immediate parent of that commit gates green and91f1e7c82itself gates red.The shadow stack is a side list of pointers that the garbage collector walks to find values held in local variables. Before #7088, binding a local into that list went through the
js_shadow_slot_bindandjs_shadow_slot_setruntime functions, and the incremental-mark root-shading barrier lived inside those functions. #7088 emits the shadow-slot store, and its barrier, as inline intermediate representation instead.Why the static barrier count jumped
The root-shading barrier was always being emitted. Before #7088 it lived inside a runtime function, so the harness's static call counter, which only scans the caller's intermediate representation, never saw it. Inlining exposed the
js_write_barrier_root_nanboxcall site directly in the workload's own code, and the static count went up accordingly.For
h1_native_rep_equivalencethe count went from 0 to 3, one barrier for each rootedBufferlocal (src,dst, andsame).h1_buffer_alias_negativerose to 46 andscalar_replacement_literalsrose to 22. Each of those exceeded the workload's heap-barrier budget, so the gate failed.Why the region labels went blank
The
native-region-proofsuite selects the loop bodies it wants to inspect by exact basic-block label, and those labels are numbered by a deterministic per-function counter. The inline lowering from #7088 inserts a run of new blocks (ss.slow,chk_top,chk_len,store,done, andbarrier) ahead of the module-initialization loops, which shifts that counter by 12.The result is that the
direct_bounded,local_cast, andhelper_indexregions moved fromfor.body.2,for.body.6, andfor.body.10tofor.body.14,for.body.18, andfor.body.22. The exact-label selectors stopped matching anything, so the suite reportedlabels=[]for those regions.Evidence that the output change is intended, not a regression
The root-shading barriers sit in guarded
ss.barrierblocks behindPERRY_INCREMENTAL_MARK_BARRIER_ACTIVE_COUNT, and they never fire in these workloads. The runtime counterwrite_barriers_tracedstays at 0 throughout.Every
js_write_barrier_root_nanboxcall is at a root-bind site, never inside a native loop. The native loops still carry rawload i8andstore i8instructions annotated with!alias.scopeand!noalias, with no runtime calls at all. This was verified in the after-optimization intermediate representation for bothh1_native_rep_equivalenceandh1_buffer_alias_negative; the latter carries 46 root barriers and zero heapjs_write_barrierorjs_write_barrier_slotcalls.#7088's own changelog states that the inline barrier is "observationally identical, not a weaker barrier" than the call it replaced. Setting
PERRY_INLINE_SHADOW_SLOT=0, the built-in revert switch, makes the affected workloads gate green again, which confirms the entire delta comes from #7088's inline lowering.Real regressions are still caught after this change. Heap barriers are still counted against the budget, and a root barrier that actually fires is caught by the
write_barriers_tracedbudget of 0. The change is monotonic: static counts can only go down, so nothing that passes today can start failing.Verification runs
python3 -m unittest tests.test_compiler_output_regression tests.test_native_abi_evidence_reportreports 87 passing, and thewrite_barriers_staticsplit is now pinned by an assertion in that suite.On the
native-region-proofgate,h1_native_rep_equivalenceandscalar_replacement_literals, the two workloads free of local-environment noise, both gate green. The region labels resolve tofor.body.14,for.body.18, andfor.body.22, and the measuredwrite_barriers_staticis 0.Running the full suite locally, HEAD plus this fix now matches the #7088 parent commit, which is the last known continuous-integration-green point. The only residual failures are pre-existing local macOS, clang 16, arm64 noise: an undeclared
@llvm.smax.i32intrinsic and a clang loop-safepoint placement difference. Both fail identically on the green parent commit and neither occurs on the Ubuntu x86-64 runner. Nowrite_barriers_static,labels=[], or root-barrier failures remain.cargo test -p perry-codegen --libreports 406 passing.One unrelated red gate is worth flagging so it is not mistaken for fallout from this change. On a clean
origin/main,cargo test -p perry-codegen'smanifest_consistencyintegration test already fails, caused by drift betweenAPI_MANIFESTandNATIVE_MODULE_TABLEincrates/perry-api-manifest/src/entries.rs.