-
-
Notifications
You must be signed in to change notification settings - Fork 155
fix(codegen): statepoint report counted zero safepoints since #7348 #7368
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| ### Fixed | ||
|
|
||
| **`--statepoint-report` reported `0 statepoints emitted` for every compile since #7348.** | ||
|
|
||
| #7348 deleted the explicit statepoint bridge, and with it the only callers of | ||
| `FunctionRecord::note_statepoint` and `note_skipped` — they lived in the bridge, | ||
| which counted safepoints as it emitted them. The methods survived with no | ||
| callers, so `statepoints`, `relocations`, `max_live_roots`, | ||
| `skipped_non_safepoints`, `live_roots_histogram` and both by-callee maps became | ||
| structurally zero in production. Measured on a real compile: | ||
|
|
||
| ``` | ||
| 5 function(s), 5 bound native root slots (5 logical slots reserved) | ||
| 5 textual calls: 5 with live roots, 0 without | ||
| 0 statepoints emitted; 0 non-collecting calls skipped <-- binary had 120 | ||
| 0 relocations; maximum 0 live roots at one safepoint | ||
| ``` | ||
|
|
||
| Counting at IR-emission time cannot work any more, and that is the real lesson: | ||
| **Perry no longer decides which calls become safepoints — `RewriteStatepointsForGC` | ||
| does, inside LLVM.** The only honest source is the compact-map rewrite, which | ||
| already parses the assembly LLVM actually emitted and computed exactly these | ||
| numbers before throwing them at `log::debug!`. The report now reads from there: | ||
|
|
||
| ``` | ||
| 120 safepoints across 6 function(s) in 1 module(s) | ||
| 36 live roots recorded, 0.30 per safepoint | ||
| ``` | ||
|
|
||
| An absent measurement no longer renders as a measured zero. `gc_map.modules == 0` | ||
| means "the rewrite never reported", the text report says | ||
| `Safepoint counts UNAVAILABLE` instead of printing zeros, and the JSON carries | ||
| `gc_map` separately from `totals` so a consumer can tell the two apart. | ||
| `schema_version` is now `2`. | ||
|
|
||
| **The CI gate now asserts the counts, not just the label.** `gc-native-roots` | ||
| checked `--only-backend rs4gc`, which passed throughout the regression — the | ||
| backend label was correct, the numbers were fiction. It now also requires | ||
| `records > 0` and `roots > 0`. Verified against a synthetic report with the | ||
| #7348 shape: the label check reports 9 functions green while the count checks | ||
| exit 1. | ||
|
|
||
| Note this is the *second* round of dead counters in this file (#7362 removed four | ||
| that never had a writer at all). The new test documents why the first invariant | ||
| missed this one: `every_rendered_counter_has_a_writer` called the mutators | ||
| itself, so "has a writer" passed while "is written" was false. The structural fix | ||
| is that the numbers now have exactly one producer and their absence is loud. | ||
|
|
||
| Three review fixes on top of the above: | ||
|
|
||
| - The report assertion ran on `09_try_catch_roots`, which contains four `try` | ||
| blocks — and RS4GC cannot rewrite WinEH funclet pads, so | ||
| `rs4gc_funclet_refusal` rejects that probe on `windows-msvc`. The probe loop | ||
| above tolerates it by grepping the compile log for "funclet"; this step did | ||
| not. The portable assertion now uses `11_collect_at_depth` (no `try`, compiles | ||
| on all four arms) and `09_try_catch_roots` keeps its own non-Windows step, so | ||
| the try-specific coverage is not lost. | ||
| - The `gc_map` doc claimed `records`/`roots` would be *absent* when unmeasured. | ||
| They are plain `u64` fields on a plain derive and always serialise; `modules` | ||
| is the sentinel. Corrected to describe what the code does. | ||
| - The "map never reported" guard in `statepoint_report_assert.py` fired for any | ||
| `--require-*`/`--print`, including fields that live in `totals` and are | ||
| counted at IR-emission time regardless of the rewrite. It is now scoped to | ||
| map-backed fields, so `--require-positive textual_calls` is answered from its | ||
| measured value instead of being failed by an unreported map it does not use. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| ### Changed | ||
|
|
||
| **Two runtime helpers admitted to the GC-effect allowlist — and a measured null result on binary size.** | ||
|
|
||
| `js_gc_register_global_root` was the single most frequent non-leaf callee in the | ||
| probe suite (148 call sites), and it is provably GC-leaf. Its entire body is: | ||
|
|
||
| ```rust | ||
| runtime_write_barrier_root_heap_word(*root); // shade one header | ||
| GLOBAL_ROOTS.with(|r| r.borrow_mut().push(root)); // TLS Vec push | ||
| ``` | ||
|
|
||
| The first call is exactly what `js_write_barrier_root_heap_word` — already | ||
| `CannotCollect` — wraps in one line. The second is a `Vec::push`, and the | ||
| "malloc count threshold" GC trigger does not apply to it: that counter is | ||
| `MALLOC_STATE.objects.len()`, a registry of Perry GC objects, and the | ||
| `#[global_allocator]` is plain mimalloc/System with no GC hook. | ||
| `js_typed_feedback_maybe_dump_trace` joins its already-admitted family siblings | ||
| (env read, JSON serialise, file write; empty body without `diagnostics`). | ||
|
|
||
| **The result, measured A/B on the same tree, is that this buys nothing:** | ||
|
|
||
| | probe | safepoints | roots | total bytes | `__text` | | ||
| |---|---:|---:|---:|---:| | ||
| | `06_string_retention` | 105 → 100 | 27 → 27 | 0 | −4 B | | ||
| | `09_try_catch_roots` | 343 → 339 | 259 → 259 | 0 | −4 B | | ||
| | `11_collect_at_depth` | 120 → 117 | 36 → 36 | 0 | −4 B | | ||
|
|
||
| Root counts are **identical**. The 40 safepoints removed across the suite were | ||
| all rootless, and a safepoint with no live roots costs essentially nothing — | ||
| which is precisely what `docs/engine-plan.md` already says ("Statepoints have no | ||
| fixed cost… the axis is not 'statepoints are bigger', it is 'roots are bigger'"). | ||
|
|
||
| This is worth recording as evidence rather than a win: **the safepoint-count | ||
| lever is not the binary-size lever.** Sequencing step 2's "reduce root density" | ||
| must attack live-root *sets*, not safepoint counts. Anyone reaching for the next | ||
| obvious helper should read the second test below first. | ||
|
|
||
| Two tests come with it. `register_global_root_tracks_the_barrier_it_wraps` pins | ||
| the two classifications together so a future demotion of the barrier cannot | ||
| leave its wrapper claiming to be leaf. `allocating_helpers_are_not_cannot_collect` | ||
| pins `js_nanbox_string` **out** of the allowlist: at 120 call sites it is the | ||
| obvious next candidate and it reads as pure bit manipulation, but its | ||
| null-pointer guard calls `js_string_from_bytes` to allocate an empty string | ||
| rather than boxing null. | ||
|
|
||
| Probe suite: 11/11 byte-identical to the Node oracle under `PERRY_RS4GC=1 | ||
| PERRY_GC_FORCE_EVACUATE=1 PERRY_GC_VERIFY_EVACUATION=1`. |
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.