feat(codegen): turn the segment-view lowering on by default - #9912
Draft
proggeramlug wants to merge 3 commits into
Draft
feat(codegen): turn the segment-view lowering on by default#9912proggeramlug wants to merge 3 commits into
proggeramlug wants to merge 3 commits into
Conversation
added 3 commits
September 6, 2026 20:27
`PERRY_SEGVIEW` is not a diagnostic — it changes the emitted code — and it is in neither the build-cache fingerprint nor any object-cache key. So a cached build can return a binary compiled with the OTHER setting: compile a source with the tier on, compile it again with the tier off, and the second can be served from the first. That is precisely the shape every A/B in this campaign uses — one compiler binary, two compiles of one source differing only in this variable — so the failure mode is not a broken build, it is two arms that are secretly the same binary and a measured difference of zero, or two arms swapped. Silent, and it would look like a result. `PERRY_SEGVIEW_DIAG` was already excluded for the weaker reason that a cached build prints no report. The switch that changes codegen was not, which is the worse omission of the two and mine. Excluded rather than keyed because the tier is experimental and default OFF. A cache key is the right fix when it ships on by default; exclusion is correct now and cannot produce a stale entry that becomes the measurement.
The cursor local is declared in the ENCLOSING statement list, not inside the
loop:
let __segview_recv = <receiver>
let __segview_input = <input>
let __segview_cursor = js_segments_view_open(recv, inp)
let __segview_iter = cur != 0 ? undefined : GetIterator(...)
For { ... } <- last read of the cursor
so without a clear its slot stays a live GC root until the function returns.
The cursor holds the input string in a traced slot, so a cursor promoted during
the loop drags that string into the old generation, and leaving the slot rooted
afterwards keeps a DEAD cursor doing it for the rest of the function.
`string-width` is entered thousands of times per reply.
That is a candidate mechanism for the idle behaviour measured on cc: I4 settles
45-65 MB ABOVE I3 at 3300 and 15-20 MB at 400 after 120 s, despite winning
20-50 MB of PEAK RSS in 12/12 paired runs. Lower peak with a higher floor is
not "less garbage"; it is something being retained.
One unconditional `LocalSet(cursor, undefined)` after the loop covers both
paths: on the declined path the local holds `0.0`, a number, so the clear is a
no-op. `break` reaches it; `return` inside the body pops the frame, which is
equally fine.
WHAT THIS DOES NOT DO, stated so the commit is not read as a cure: it does not
prevent promotion DURING the loop, and nothing in the compiler can, because the
cursor is genuinely live there. It removes only the post-loop rooting of a dead
cursor. If the idle delta comes from cursors promoted mid-loop, this will not
move it. perrymaster's old-gen census after idle, counting class id
0xFFFF_000E on I5-spec / I5-view / I7-view, decides that independently.
The test is structural rather than string-matched: it locates the rewritten
`For`, reads the cursor's LocalId out of the loop's own guard, and requires the
next statement to be `LocalSet(that id, Undefined)`. It fails if the clear is
removed, clears the wrong local, or is emitted before the loop.
17/17 segview tests.
The tier shipped default OFF because the runtime's view entry points did not exist. PerryTS#9870 landed them on main, and on claude-code the tier with PerryTS#9893's levers is -14 % CPU and -30…-42 MB peak RSS across 5/5 paired runs with identical output. `PERRY_SEGVIEW=0` remains as the opt-out. A switch that changes emitted code needs an off position that does not require rebuilding the compiler, and a program whose loops the tier declines pays nothing either way. The test asserts the default through the same environment read the compiler uses. Without it, a predicate that was accidentally always-false would leave every other segview test passing — they call the rewrite directly — while the tier silently never fired in a real compile.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
jdalton
reviewed
Sep 7, 2026
jdalton
left a comment
jdalton
left a comment
Contributor
There was a problem hiding this comment.
Review of 9b9aedda258ea678019667c4257f9642ffe1e8ce (2026-09-07).
Please add a compiled cache A/B regression for unset → PERRY_SEGVIEW=0 → unset, checking emitted calls/output rather than just the environment predicate. The new eligibility exclusion only covers the build-cache layer, so the object-cache fingerprint also needs to distinguish the setting; the comment still says the feature is default OFF and should be corrected for this PR. For the new cursor clear, add abrupt-control-flow cases (break and a labelled outer continue, plus a caught throw). A statement after the loop handles ordinary exit, but not every path that can keep the enclosing function/frame alive. No demonstrated semantic miscompile from the inspected diff.
Validation scope: source/diff inspection; I have not run this PR's build or test suite locally.
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.
DRAFT. Gated on two things, neither of which is done:
PERRY_SEGVIEWinto the build-cacheidentity. Flipping the default while the switch is absent from that identity
would bake the hole into every build rather than every A/B — a cached
binary could be served for a source compiled under the other setting. fix(compile): put PERRY_SEGVIEW in the cache identity, and clear the segment-view cursor at loop exit #9910
is also the parent of this branch, so this diff is the one-line flip on top
of it.
set
PERRY_SEGVIEW=1explicitly. Default-on is a different configuration —it is what every compile in the tree does, including ones nobody chose it
for — and it has not been rotated. Until that row exists this is unmeasured
in the form it would ship.
The change
One line, plus the doc comment it invalidates:
PERRY_SEGVIEW=0becomes the opt-out. It stays because a switch that changesemitted code needs an off position that does not require rebuilding the
compiler; a program whose loops the tier declines pays nothing either way.
Why the default should move, once gated
On claude-code, with #9893's levers, paired against main on a quiet box:
−14 % CPU MIN (5/5 faster), −30…−42 MB peak RSS (5/5), settled RSS flat,
output identical; 4.8–5.3× node from main's 5.6–6.3×. On the probe's region A
— cc's per-
.segment()-call shape — 2,262 → 1,554 ns/grapheme against node's1,323, i.e. 1.17× node. Rows and their provenance are in #9859.
Without #9893's levers the same tier costs +15 % CPU, which is why the
gating on it is not a formality.
The test
Asserts the default through the same environment read the compiler uses: unset
⇒ on,
0⇒ off,1⇒ on. Every other segview test calls the rewrite directly,so a predicate that was accidentally always-false would leave all of them
passing while the tier silently never fired in a real compile — the #9824
shape, and the reason this assertion is worth its three lines.
18 segview tests pass on this branch.
Default-on gate on main
504e180d0(perrymaster, cc 3300-char reply, 5-round interleaved rotation; paired deltas)Identity from the artefacts: with
PERRY_SEGVIEWunset and no diag, the branch rewrites the same nine sites as the opt-in build (view call sites next/open 20/10 vs 0/0 on main), so the flip does exactly what the env switch did.400-char turn: default-on alone +0.09…+0.20 s (3/3 slower), with the levers −0.05…−0.11 s (3/3 faster); peak −9…−35 MB either way.
Consequence: this flip must land together with or after #9893. On its own it fails the campaign's CPU rule (+3…+5 % at 3300, +7…+14 % at 400) while buying peak RSS; with the levers it is the I7-view result on the current base. Absolute numbers on this host currently carry a ~35 % shift from a foreign service; the pairs above are interleaved and hold. Raw:
combDO.jsonl,idleDO.jsonl.