bytecode: avoid quadratic work in basic block linking and liveness propagation - #356
bytecode: avoid quadratic work in basic block linking and liveness propagation#356robobun wants to merge 1 commit into
Conversation
…opagation
A deeply nested array destructuring pattern emits O(depth) basic blocks,
O(depth) exception handlers, and O(depth) callee locals: each nesting
level wraps IteratorClose in a synthesized try/finally. When such a
pattern sits inside a try/catch, the first time the catch executes LLInt
calls ensureCatchLivenessIsComputedForBytecodeIndex, which builds the
bytecode graph and runs the liveness fixpoint over the whole code block.
At N=3000 that takes several seconds; debug+ASAN runs for minutes.
Two separate quadratic costs showed up under profiling:
1. BytecodeBasicBlock::computeImpl linked each branch and throw to its
successor block by scanning the entire basic block list per branch.
Blocks are already sorted by leaderOffset (they are created in
program order; BytecodeGraph::findBasicBlockWithLeaderOffset already
relies on that ordering), so replace the linear scan with
tryBinarySearch. addSuccessor() already dedupes, which covers the
switch case where one block appears more than once among the branch
targets.
2. BytecodeLivenessPropagation::stepOverInstruction called
handlerForBytecodeIndex (a linear scan over all handlers) and then
forEachSetBit over the handler block's live-in set, setting one bit
at a time, for every instruction, on every fixpoint iteration.
Exception handler start/end/target offsets are added to the precise
jump target set by computePreciseJumpTargets, so a handler range
cannot start or end in the middle of a basic block, and every
instruction in a block therefore resolves to the same innermost
handler. Hoist the handler lookup to once per block, precompute the
handler-block mapping once at the start of runLivenessFixpoint, and
fold the handler's live-in into `out` once with a word-wise `|=`;
keep it folded in by having the def functor leave a local alone when
the handler keeps it live, instead of re-applying the merge after
every def.
For `try { let [[[...2500 deep...]]] = w } catch (e) {}` this brings
the first-catch cost from ~4s to ~0.5s in a release build.
dumpBytecodeLivenessResults output is unchanged on code exercising
nested try/catch and array destructuring.
|
Warning Review limit reached
Next review available in: 12 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 (4)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| case op_iterator_open: { | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🔴 This removes the case op_async_iterator_open: branch, but that opcode has two checkpoints (symbolCall, getNext) — when tmpLivenessForCheckpoint is called at checkpoint 1 the !checkpoint early-return is skipped and execution now falls through to RELEASE_ASSERT_NOT_REACHED(). Any for await loop that reaches DFG and queries tmp liveness at the getNext checkpoint will crash the process; the case (which just returns the empty result, same as op_iterator_open) should be restored.
Extended reasoning...
What was removed
The diff deletes:
case op_async_iterator_open: {
return result;
}from the switch in tmpLivenessForCheckpoint(). This function is unrelated to the basic-block linking / liveness-fixpoint work described in the PR — it computes checkpoint-tmp liveness for DFG, not local liveness — so the deletion looks accidental (perhaps a stray hunk from an editor cleanup).
Why it crashes
op_async_iterator_open is defined in BytecodeList.rb:231-250 with two checkpoints:
checkpoints: {
symbolCall: nil,
getNext: nil,
}so numberOfCheckpoints() == 2. tmpLivenessForCheckpoint begins with:
if (!checkpoint)
return result;which handles checkpoint 0 (symbolCall). For checkpoint 1 (getNext) that guard is skipped and the switch is entered. With the op_async_iterator_open case removed, none of the remaining cases match, execution falls to default: break;, and the function hits RELEASE_ASSERT_NOT_REACHED().
Step-by-step
- A
for await (const x of y)loop emitsop_async_iterator_open(BytecodeGenerator). - The loop tiers up to DFG.
DFGByteCodeParser.cpphandlesop_async_iterator_openviahandleAsyncIteratorOpen, so it is DFG-compilable. - Something asks for tmp liveness at the
getNextcheckpoint — e.g.DFGForAllKills.h:84-94,DFGGraph.h:1164(forAllLiveInBytecode),DFGGraph.cpp:1205, orDFGByteCodeParser.cpp:696, all of which calltmpLivenessForCheckpoint(codeBlock, bytecodeIndex)wheneverbytecodeIndex.checkpoint() != 0. checkpoint == 1→!checkpointis false → switch onop_async_iterator_open→ no case matches →default: break;→RELEASE_ASSERT_NOT_REACHED()→ process crash.
Why nothing else prevents it
The only guard before the switch is if (!checkpoint) return result;, which by design only covers checkpoint 0. Every multi-checkpoint opcode must therefore appear explicitly in the switch. op_iterator_open (the sync twin, same two checkpoints) is still present; op_async_iterator_open was present in the base commit (HEAD~1 line 195) and is removed only by this PR.
Impact
This is a release-build crash (RELEASE_ASSERT_NOT_REACHED), not a debug-only assert. Any async-iteration-heavy code that warms up enough to reach DFG will terminate the process. It is a strict regression introduced by this PR and unrelated to its performance goal.
Fix
Restore the deleted case (it can share the op_iterator_open arm since both return the empty bitset):
case op_iterator_open:
case op_async_iterator_open: {
return result;
}
Preview Builds
|
2 participants
A deeply nested array destructuring pattern emits O(depth) basic blocks, O(depth) exception handlers, and O(depth) callee locals (each nesting level wraps IteratorClose in a synthesized try/finally). When such a pattern sits inside a
try/catch, the first time the catch executes LLInt callsensureCatchLivenessIsComputedForBytecodeIndex, which builds the bytecode graph and runs the liveness fixpoint over the whole code block.Release
jscbefore this change:Two separate quadratic costs showed up under sampling:
BytecodeBasicBlock::computeImplLinked each branch and throw to its successor block by scanning the entire basic block list per branch. Blocks are already sorted by
leaderOffset(they are created in program order, andBytecodeGraph::findBasicBlockWithLeaderOffsetalready binary-searches over that ordering), so replace the linear scan withtryBinarySearch.addSuccessor()already dedupes, which covers the switch case where one block appears more than once among the branch targets.BytecodeLivenessPropagation::stepOverInstructionCalled
handlerForBytecodeIndex(a linear scan over all handlers) and thenforEachSetBitover the handler block's live-in set, setting one bit at a time, for every instruction, on every fixpoint iteration.computePreciseJumpTargetsadds every handler'sstart,end, andtargetto the precise jump target set, so a handler range cannot start or end in the middle of a basic block, and every instruction in a block resolves to the same innermost handler. Hoist the handler lookup to once per block, precompute the handler-block mapping once at the start ofrunLivenessFixpoint, and fold the handler's live-in intooutonce with a word-wise|=; keep it folded in by having the def functor leave a local alone when the handler keeps it live, instead of re-applying the merge after every def.Result
Release
jscwith this change:The remaining growth is the inherent O(blocks x numCalleeLocals / 64) bitvector dataflow in the liveness fixpoint itself.
Verification
--dumpBytecodeLivenessResults=trueoutput is byte-identical before and after on a script exercising nested try/catch, nested array destructuring, and for-of. 80 JSC stress tests coveringcatch/try/destructur/for-of/iterator/finally/livenessproduce identical output and exit codes against an unpatched build.This is complementary to #355, which adds a native-stack guard in
ArrayPatternNode::bindValue; the depths in the table above are within that guard's budget on a release build.