Skip to content

DFG: stop rooting m_mustHandleValues from the concurrent compiler plan #308

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.

Sign up for GitHub

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

Open
robobun wants to merge 1 commit into
base: pin/4895f45d
Choose a base branch
from
Open

DFG: stop rooting m_mustHandleValues from the concurrent compiler plan #308

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 23 additions & 5 deletions Source/JavaScriptCore/dfg/DFGPlan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,23 @@
ASSERT(m_vm);
if (m_recordedStatuses)
m_recordedStatuses->finalizeWithoutDeleting(*m_vm);

// m_mustHandleValues was snapshot from the triggering frame's live locals
// so the compiler can seed OSR-entry predictions. It is not a reason to
// keep those objects alive on its own: the DFG phases that read it all
// treat nullopt as "unknown", so any entry that nothing else marked can be
// dropped here (compiler threads are suspended for the duration of GC).
cleanMustHandleValuesIfNecessary();
{
Locker locker { m_mustHandleValueCleaningLock };
for (unsigned i = m_mustHandleValues.size(); i--;) {
std::optional<JSValue>& value = m_mustHandleValues[i];
if (!value || !*value || !value->isCell())
continue;
if (!m_vm->heap.isMarked(value->asCell()))
value = std::nullopt;
}
}
Comment thread
claude[bot] marked this conversation as resolved.
}

void Plan::notifyReady()
Expand Down Expand Up @@ -658,12 +675,13 @@
if (!Base::checkLivenessAndVisitChildren(visitor))
return false;

// m_mustHandleValues is not visited here. It holds whatever JSValues were
// live in the frame at the tier-up point, purely to seed OSR-entry
// predictions; rooting them would keep arbitrary user objects alive for the
// life of a concurrent compile. finalizeInGC() nulls entries that nothing
// else marked, and every reader (PredictionInjection, CFA injectOSR,
// TypeCheckHoisting) already skips nullopt.
cleanMustHandleValuesIfNecessary();

Check warning on line 684 in Source/JavaScriptCore/dfg/DFGPlan.cpp

View check run for this annotation

Claude / Claude Code Review

Vestigial cleanMustHandleValuesIfNecessary() in checkLivenessAndVisitChildren() 

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.

@claude claude Bot Jul 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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:

  1. compileInThreadImpl() — cleans at the very top before any DFG phase reads m_mustHandleValues.
  2. finalizeInGC() — cleans immediately before its own isCell()/isMarked() loop.
  3. checkLivenessAndVisitChildren() (this line) — used to clean immediately before the appendUnbarriered loop 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

  1. A plan is enqueued with m_mustHandleValuesMayIncludeGarbage == true; the compiler thread hasn't picked it up yet.
  2. GC starts. During the marking fixpoint, the Jw constraint calls checkLivenessAndVisitChildren(). Base::checkLivenessAndVisitChildren returns true → line 684 runs cleanMustHandleValuesIfNecessary(), clearing the flag and nulling dead-slot entries. Nothing in this function then reads the result.
  3. Post-mark, removeDeadPlans() calls finalizeInGC() on the surviving plan. finalizeInGC() calls cleanMustHandleValuesIfNecessary() again → early-returns because the flag is already clear → runs its isMarked loop on cleaned values.
  4. 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 that finalizeInGC() 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.

for (unsigned i = m_mustHandleValues.size(); i--;) {
std::optional<JSValue> value = m_mustHandleValues[i];
if (value)
visitor.appendUnbarriered(value.value());
}

if (m_recordedStatuses) {
m_recordedStatuses->visitAggregate(visitor);
Expand Down
Loading