DFG: stop rooting m_mustHandleValues from the concurrent compiler plan#308
DFG: stop rooting m_mustHandleValues from the concurrent compiler plan#308robobun wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Preview Builds
|
06ecce0 to
14b1b4a
Compare
|
Rebased onto |
When a hot loop triggers Baseline->DFG (or DFG->FTL) OSR, the tier-up
path snapshots every live argument and local of the triggering frame
into Plan::m_mustHandleValues so the compiler can seed OSR-entry type
predictions. Plan::checkLivenessAndVisitChildren marked those JSValues
as roots for the life of the concurrent compile, which meant whatever
user objects happened to be in scope at the tier-up point stayed alive
until the plan was finalized. In Bun this showed up as Node's
test-gc-http-client* suite reporting N-1/N collected: an in-flight plan
was rooting one IncomingMessage (and the ClientRequest it points to)
via RootMarkReason::JITWorkList.
The compiler does not need those values kept alive on its own. Every
reader of m_mustHandleValues (PredictionInjection, CFA injectOSR,
TypeCheckHoisting) already skips nullopt. So:
- stop visiting m_mustHandleValues in checkLivenessAndVisitChildren
- in finalizeInGC (post-mark, compiler threads suspended), drop any
entry whose cell was not otherwise marked
Values that survive marking (still reachable from elsewhere, or already
frozen into the Graph at an earlier safepoint) are unaffected. Values
that are only reachable via the plan become nullopt; the OSR entry for
that local simply widens to unknown.
14b1b4a to
40ff52a
Compare
|
Amended with a fix found by bun's CI: |
There was a problem hiding this comment.
Both earlier findings are addressed — the !*value guard covers the TDZ/empty-JSValue → isMarked(nullptr) crash, and finalizeInGC() now cleans for itself so the garbage-window invariant is self-contained. The one remaining inline note is a cosmetic nit (the now-redundant clean call in checkLivenessAndVisitChildren); it's harmless either way.
I'm still deferring to a human on this one: it changes GC-rooting semantics on the concurrent-JIT path, and the correctness argument leans on invariants (compiler threads suspended at finalizeInGC, every reader either tolerating nullopt or having already graph.freeze()d what it read before the next safepoint) that are worth a second pair of JSC eyes.
Also checked: mergeOSREntryValue freezes/registers the cell before storing it in AbstractValue::m_value, so CFA-injected values that persist in block state across a safepoint are covered by m_weakReferences.visitChildren and won't be swept out from under the graph.
Extended reasoning...
Overview
The PR touches a single file, DFGPlan.cpp, but the change is semantically deep: it stops the concurrent DFG/FTL compiler plan from acting as a GC root for m_mustHandleValues (the snapshot of live locals at the OSR tier-up point), and instead treats those entries as weak — nulling any unmarked cell in finalizeInGC() post-mark. The motivation (Bun's test-gc-http-client* suite showing objects pinned by RootMarkReason::JITWorkList) and verification are well-documented.
The first revision had a real crash (isMarked(nullptr) via engaged-but-empty TDZ entries) and a fragile cross-function coupling; the amendment fixed both by adding the !*value guard and calling cleanMustHandleValuesIfNecessary() locally in finalizeInGC(). The only remaining finding this run is a harmless nit about the now-vestigial clean call left behind in checkLivenessAndVisitChildren().
Security risks
None in the traditional sense (no auth/injection/data exposure). The relevant risk class here is memory safety: weakening a GC root can produce use-after-free if any consumer holds a raw cell pointer across a safepoint without separately rooting it. I traced the three documented readers — PredictionInjectionPhase only extracts a SpeculatedType bitmask; CFAPhase::injectOSR goes through AbstractValue::mergeOSREntryValue, which graph.freeze()s the value and registerStructure()s its structure before storing m_value, so the cell is tracked via m_weakReferences.visitChildren; TypeCheckHoistingPhase reads structure() within a single phase (no intervening safepoint). That matches the PR description's "already frozen into the Graph at an earlier safepoint" claim, but this is exactly the kind of invariant chain a JSC-fluent human should confirm.
Level of scrutiny
High. This is core GC-marking / concurrent-JIT interaction in JavaScriptCore — one of the most subtle parts of the engine. It also diverges from upstream WebKit behavior. The first revision shipped with a null-deref reachable from ordinary let/const code, which reinforces that this path deserves careful human review even though the diff is small.
Other factors
- The author responded to both prior review points in the amendment and verified against Bun's ASAN build plus the original GC fixture.
finalizeInGC()runs with compiler threads suspended (per the JITWorklistremoveDeadPlanspath), so the lock + mutation there is race-free; theLockeris belt-and-suspenders.- The remaining nit (idempotent
cleanMustHandleValuesIfNecessary()under a "not visited here" comment) is purely cosmetic and shouldn't block. - No test changes in this repo; the regression test lives in the companion Bun PR.
| // life of a concurrent compile. finalizeInGC() nulls entries that nothing | ||
| // else marked, and every reader (PredictionInjection, CFA injectOSR, | ||
| // TypeCheckHoisting) already skips nullopt. | ||
| cleanMustHandleValuesIfNecessary(); |
There was a problem hiding this comment.
🟡 nit: now that finalizeInGC() calls cleanMustHandleValuesIfNecessary() itself, this call has no reader — the appendUnbarriered loop it used to serve is gone, and both remaining consumers (finalizeInGC, compileInThreadImpl) clean for themselves. It's harmless (idempotent, early-returns on the flag), but keeping a call that mutates m_mustHandleValues state directly under a comment saying "m_mustHandleValues is not visited here" reads confusingly; consider dropping it.
Extended reasoning...
What changed
The earlier review round noted that the cleanMustHandleValuesIfNecessary() call in checkLivenessAndVisitChildren() "looks vestigial, yet is load-bearing" because finalizeInGC() was relying on it to have cleaned the values before its isCell()/isMarked() loop ran. The amendment addressed that by making finalizeInGC() self-contained: it now calls cleanMustHandleValuesIfNecessary() directly (DFGPlan.cpp:169) before iterating. That resolves the load-bearing part — but leaves the original call site behind with nothing depending on it.
Why it is now dead
There are exactly three call sites for cleanMustHandleValuesIfNecessary() after the amendment:
compileInThreadImpl()— cleans at the very top before any DFG phase readsm_mustHandleValues.finalizeInGC()— cleans immediately before its ownisCell()/isMarked()loop.checkLivenessAndVisitChildren()(this line) — used to clean immediately before theappendUnbarrieredloop that this PR deleted.
Site (3) no longer has a local consumer: nothing in checkLivenessAndVisitChildren() reads m_mustHandleValues anymore. And it no longer has a downstream dependent: the only GC-side reader is finalizeInGC(), reached via removeDeadPlans() for every plan whose isKnownToBeLiveAfterGC() is true, and that path now cleans for itself. Cancelled plans don't matter (their m_mustHandleValues is cleared in cancel()).
Why it is harmless
cleanMustHandleValuesIfNecessary() is idempotent — it takes m_mustHandleValueCleaningLock, checks m_mustHandleValuesMayIncludeGarbage, and returns immediately if the flag is already clear. So whichever of the three sites runs first does the work and the others become a lock-acquire-and-return. There is no correctness or performance concern; this is purely a readability cleanup.
Step-by-step walkthrough
- A plan is enqueued with
m_mustHandleValuesMayIncludeGarbage == true; the compiler thread hasn't picked it up yet. - GC starts. During the marking fixpoint, the
Jwconstraint callscheckLivenessAndVisitChildren().Base::checkLivenessAndVisitChildrenreturns true → line 684 runscleanMustHandleValuesIfNecessary(), clearing the flag and nulling dead-slot entries. Nothing in this function then reads the result. - Post-mark,
removeDeadPlans()callsfinalizeInGC()on the surviving plan.finalizeInGC()callscleanMustHandleValuesIfNecessary()again → early-returns because the flag is already clear → runs itsisMarkedloop on cleaned values. - If step 2 were removed, step 3 would do the cleaning itself with identical effect. If the compiler thread had already started,
compileInThreadImpl()would have cleaned first and both GC-side calls would early-return. In every ordering, the call at line 684 either does work thatfinalizeInGC()would do anyway, or is a no-op.
Why it's worth mentioning
The call sits directly under a six-line comment whose thesis is "m_mustHandleValues is not visited here" — and then the very next statement takes a lock and mutates m_mustHandleValues state. A reader has to go check that cleanMustHandleValuesIfNecessary() doesn't visit anything (it doesn't) and then wonder why it's called at all. Dropping the call — or at minimum removing it and letting the comment stand alone — would make the function match its own documentation.
Suggested fix
Delete the cleanMustHandleValuesIfNecessary(); line from checkLivenessAndVisitChildren(). The explanatory comment above it already documents why nothing here touches m_mustHandleValues, and both actual readers are self-contained.
…) and trim comments oven-sh/WebKit#308 is now based on 4895f45d (the currently pinned commit) so the preview artifact contains only the DFGPlan change, and finalizeInGC skips jsEmpty() entries (a TDZ-sentinel or LLInt-zeroed temp at the OSR point satisfies isCell() but asCell() is null).
1 participant
When a hot loop triggers Baseline->DFG (or DFG->FTL) OSR, the tier-up path snapshots every live argument and local of the triggering frame into
Plan::m_mustHandleValuesso the compiler can seed OSR-entry type predictions.Plan::checkLivenessAndVisitChildrenmarked those JSValues as roots for the life of the concurrent compile, so whatever user objects happened to be in scope at the tier-up point stayed alive until the plan was finalized.In Bun this surfaces as Node's
test-gc-http-client*suite reporting N-1/N collected while plans are queued: a GCDebugging heap snapshot at that point shows oneIncomingMessage(and theClientRequestit points to) rooted directly withRootMarkReason::JITWorkList. WithuseConcurrentJIT=0oruseDFGJIT=0the same program collects in one gc.The compiler does not need those values kept alive on its own. Every reader of
m_mustHandleValues(DFGPredictionInjectionPhase,DFGCFAPhase::injectOSR,DFGTypeCheckHoistingPhase) already skipsnullopt. So:m_mustHandleValuesincheckLivenessAndVisitChildrenfinalizeInGC(runs post-mark with compiler threads suspended, before sweep), drop any entry whose cell was not otherwise markedValues that survive marking (still reachable from elsewhere, or already frozen into the Graph at an earlier safepoint) are unaffected. Values reachable only via the plan become
nullopt; the OSR-entry prediction for that local simply widens to unknown, and OSR entry validation handles the rest.Verification
Built Bun with this change applied to JSC. Driver: make 32
http.getrequests, wait for all responses,Bun.gc(true)once, then take a GCDebugging heap snapshot and countClientRequest/IncomingMessagenodes whose root reason isJITWorkList.jitworklist-rootedClientRequestalive after one gcAll four
test-gc-http-client*andtest-net-connect-memleakpass cleanly. Companion Bun PR adds the regression test.