[JSC] A jettison for a dead weak reference counts toward the reoptimization back-off - #634
[JSC] A jettison for a dead weak reference counts toward the reoptimization back-off#634Jarred-Sumner wants to merge 1 commit into
Conversation
…zation back-off DFG and FTL code refers weakly to the structures, prototypes and other cells it was specialized on. When one of them dies the collector jettisons the code (Profiler::JettisonDueToWeakReference), the baseline CodeBlock is installed again and the function tiers up again. That recompile was as eager as the first compile: only jettisons for OSR exits, for the baseline loop trigger and for unprofiled watchpoints passed CountReoptimization. A function whose optimized code checks the structure of objects with a short-lived prototype is compiled, dies in the next full collection and is compiled again at full price for as long as the process lives. In a 14 minute session of a large bundled CLI application half of the DFG compile time was repeat compiles, and the largest part of those followed a weak-reference jettison. ScriptExecutable::jettisonCodeBlockEdgeIfDead() now passes CountReoptimization for such a jettison while the function's reoptimization retry counter is below Options::weakReferenceJettisonReoptimizationLimit() (default 4; 0 restores the old behaviour). Each counted death doubles the execution count the next DFG and FTL compiles wait for, like a jettison for exiting too often. The limit is there because the counter never decays and also scales the number of OSR exits tolerated before reoptimization: unbounded, a function whose code dies in every full collection of a long-lived process would end up waiting forever. The count goes through CodeBlock::countReoptimization(), so the existing clamp at reoptimizationRetryCounterMax applies. Jettisons for old age are not counted: the counter lives on the baseline CodeBlock, which an aged-out optimizing block does not keep alive, so it dies in the same collection. No code generation changes; a later compile sees the same bytecode with at least as much profiling. * JSTests/stress/weak-reference-jettison-counts-toward-reoptimization-backoff.js: Added. * Source/JavaScriptCore/runtime/OptionsList.h: * Source/JavaScriptCore/runtime/ScriptExecutableInlines.h: (JSC::ScriptExecutable::jettisonCodeBlockEdgeIfDead):
Preview Builds
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The runtime change is tiny and well-contained, but it adjusts a core JIT tier-up heuristic and both changed paths are covered by CODEOWNERS (@ WebKit/jsc-reviewers), so a human look is still worthwhile — particularly on the default of 4 and on whether the new stress test meets the JSTests <200ms / testLoopCount rules across its three runDefault configurations.
What was reviewed:
jettisonCodeBlockEdgeIfDead:shouldJettisonDueToWeakReferencegates onisOptimizingJIT, sobaselineAlternative()is always valid here;jettison()already dereferences it on theCountReoptimizationpath, so no new lifetime hazard.- Counter overflow:
countReoptimization()clamps atreoptimizationRetryCounterMax, and the new limit only gates whether to increment, so setting it to 100 cannot overflow theuint16_t. - Option plumbing: added alongside
reoptimizationRetryCounterMaxinOptionsList.hwithUnsignedtype and a description string;0correctly restores prior behavior since the comparison is strict<.
Extended reasoning...
Overview
The PR changes one call site in Source/JavaScriptCore/runtime/ScriptExecutableInlines.h: when an optimizing CodeBlock is jettisoned because a weakly-referenced cell died, the jettison now passes CountReoptimization (instead of the implicit DontCountReoptimization default) while the baseline alternative's retry counter is below a new option weakReferenceJettisonReoptimizationLimit (default 4). A new option is added to OptionsList.h, and a new stress test in JSTests/stress/ exercises the behavior across three option values (default, 0, 100) with --useConcurrentJIT=false, checking both the retry counter value and the roughly-doubling call count per life.
Security risks
None identified. No code generation, parsing, or memory-management logic is touched. The only effect is the value of m_reoptimizationRetryCounter on a baseline CodeBlock after a jettison that already happens today; every value it can now reach was already reachable via OSR-exit reoptimizations, and countReoptimization() clamps at reoptimizationRetryCounterMax. shouldJettisonDueToWeakReference returns true only for optimizing JIT code, so baselineAlternative() walks a valid chain (and jettison() itself already calls baselineAlternative()->countReoptimization() on this path, so no new dereference is introduced).
Level of scrutiny
Medium-to-high. The mechanical diff is three lines plus an option, and the reasoning in the PR description is thorough and internally consistent with the code I read (adjustedCounterValue shifts by the retry counter; numberOfDFGCompiles is retry count plus one when optimized code is installed, which matches the test's numberOfDFGCompiles(subject) !== reoptimizationRetryCount(subject) check after GC). However, this is a JIT tier-up heuristic with process-wide performance implications, the default value (4) is a judgment call backed by measurements on one workload, and both Source/JavaScriptCore/ and JSTests/ are CODEOWNER-covered. That combination puts it outside the "simple/mechanical" bar for auto-approval.
Other factors
The new stress test hard-codes maximumCallsPerLife = 1 << 20 and runs 2 scenarios × 9 lives × 3 configurations, with the limit=100 configuration doubling up to 2^7× the base threshold by life 8. JSTests/README.md requires new stress tests to run in under 200ms in all configurations and to use testLoopCount for iteration sizing; the test's design (measuring actual tier-up call counts) makes testLoopCount a poor fit, but the 200ms budget is worth confirming with --report-execution-time. The test also depends on conservative-stack-scanning behavior (clobberStack) and retries up to 3 times per scenario, which is a reasonable pattern for GC-timing-sensitive tests but adds to the case for a human sanity check.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. WalkthroughThe runtime adds a limit for weak-reference jettison reoptimization counting. A stress test covers structure- and prototype-based weak references, garbage collection, recompilation, retry counts, and capped backoff. ChangesWeak-reference reoptimization backoff
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The retry-limit behavior and stress coverage align with the intended weak-reference reoptimization backoff change. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives a detailed explanation of the change, rationale, measurements, affected jettison reasons, and testing. However, it does not include the required Bugzilla bug title and URL, review status, or the template's explicit changed-file and function list. Resolution Add the associated Bugzilla URL and bug title, include the required review line such as "Reviewed by NOBODY (OOPS!)." or the actual reviewer, and add the changed paths with relevant functions. Retain the existing technical explanation and testing details.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
1 participant
What
DFG and FTL code refers weakly to the structures, prototypes and other cells it was specialized on. When one of
them dies the collector jettisons the code (
Profiler::JettisonDueToWeakReference), the baselineCodeBlockisinstalled again,
optimizeAfterWarmUp()re-arms its counter and the function tiers up again. That recompile hasalways been as eager as the first compile:
CodeBlock::jettison()only bumps the reoptimization retry counter whenthe caller passes
CountReoptimization, andScriptExecutable::jettisonCodeBlockEdgeIfDead()never did. A functionwhose optimized code keeps dying this way (it checks the structure of objects whose prototype is created per request,
per render, per task) is compiled, runs for a few seconds, dies in the next full collection, and is compiled again at
full price, for as long as the process lives.
With this change such a jettison counts as a reoptimization while the function's retry counter is below
Options::weakReferenceJettisonReoptimizationLimit()(new, default 4; 0 restores the old behaviour). Each counteddeath doubles the execution count the next DFG compile waits for, and through
baseline->adjustedCounterValue()theFTL tier-up threshold too, exactly like a jettison for exiting too often.
The diff is one call site (
ScriptExecutableInlines.h) and one option.Why (measurements)
A large bundled CLI application (interactive terminal UI, ~10,000 linked CodeBlocks, ~3,000 of them JIT-compiled),
100-turn session paced like a person types and a server streams (14 minutes, 114 to 116 G user instructions),
perfsamples joined with the JIT dump and a per-compile log:
that had been compiled in that tier before;
1.3 + 1.0 G, the code dying with its CodeBlock 0.85 G, old age 0.2 G. Only the OSR-exit bucket is rate-limited today;
One function of 8.9 k bytecode cost was DFG-compiled 5 times at ~100 M instructions each, one FTL-compiled 4 times
at 160 to 250 M, each copy living 9 to 23 s.
Same session, with every weak-reference jettison counted (the first version of this change, no limit):
Re-measured with this change (whole-process
perf stat -e instructions:u, variants interleaved, each on its own copy ofthe binary, option toggled by environment). All application numbers in this text are from the application's build,
which also carries #628; the shell numbers and the test runs are from
mainplus this commit:| mean | 115.9 G | 114.0 G (-1.7 %) | 113.4 G (-2.2 %) | 113.2 G (-2.3 %) |
(One more pair with the first version, no limit: 116.07 G -> 112.67 G, -2.9 %. Two runs of the same variant differ by up to 0.7 G.)
Time per turn is the stream in every variant (890 to 905 s for the 100 turns); peak anonymous memory 239 to 248 MB in all.
An unpaced burst of 20 turns (one 5 s burst from cold; it has almost no weak-reference jettisons) is within noise:
20.92 / 20.39 / 21.15 G before, 20.46 / 20.46 / 20.10 G after (3 interleaved rounds); peak anonymous memory
198 to 227 MB in both.
Which jettison reasons count
JettisonDueToOSRExittriggerReoptimizationNowJettisonDueToBaselineLoopReoptimizationTrigger,...OnOSREntryFailJettisonDueToUnprofiledWatchpointCodeBlockJettisoningWatchpoint,DFG::AdaptiveStructureWatchpoint,DFG::AdaptiveInferredPropertyValueWatchpointJettisonDueToWeakReferencejettisonCodeBlockEdgeIfDead: the executable is marked, its optimizing block is not, and it did not age outvisitCodeBlockEdgeprecisely so that it can be reinstalled, andjettison()re-arms itJettisonDueToOldAgeCodeBlock, and for an aged-out optimizing blockvisitCodeBlockEdgedeliberately does not keep the alternative alive, so the counter's owner dies in the same collection. Counting would be a no-op. These recompiles are also rate-limited by construction (at most one per lease) and measured at 0.2 G of 5.2 GJettisonDueToVMTraps,JettisonDueToDebuggerBreakpoint,...SteppingWhy a limit, and why 4
The retry counter never decays, it is shared with OSR-exit reoptimizations, and it also scales the number of OSR
exits tolerated before optimized code is reoptimized (
adjustedExitCountThreshold). Weak-reference deaths saynothing about the quality of the speculation, and in a long-lived process there is no bound on how many of them a
function sees: uncapped, a request handler whose optimized code dies in every full collection would, a few dozen
collections later, wait 2^18 times the normal count and effectively stay in baseline code for good, having
tolerated up to 2^18 x 100 exits on the way. With the limit the worst case is a 16x threshold and a 16x exit
tolerance: a function that is hot recompiles within seconds of the death, a function that barely reached the
threshold between two collections (the ones that make up the repeat-compile bucket above) does not.
The counter itself cannot overflow: the count goes through
CodeBlock::countReoptimization(), which clamps atreoptimizationRetryCounterMax(derived from the largest shift that keeps the threshold inside int32; the field isa
uint16_t). A limit above that maximum just means "no limit".In the measured session a limit of 4 keeps about three quarters of what counting without a limit saves (-1.7 % against -2.3 %
of the process's instructions), 6 all of it. 4 is the cautious choice: after a death a hot function is back in DFG code after 16x
and back in FTL code after 16x the usual wait, not 64x; the option is there to move it.
This cannot cause a miscompile
No code generation is touched: not the DFG, not B3/FTL, not the baseline JIT, not OSR exit or entry, not what the
collector marks or when it jettisons. The only effect is the value of
m_reoptimizationRetryCounteron a baselineCodeBlockafter a jettison that already happened, i.e. the constantadjustedCounterValue()multiplies theexecution-count threshold with. A compile that happens later than before compiles the same bytecode with at least as
much profiling. Every value the counter can now take it could already take (an OSR-exit reoptimization increments
the same field through the same function).
Test
JSTests/stress/weak-reference-jettison-counts-toward-reoptimization-backoff.js: two functions (one checks thestructure of an object whose prototype nothing else refers to, one loads an inherited property from such a prototype)
are called until they run in DFG code, the object is dropped,
fullGC()kills the code through the dead prototype,nine times. The number of calls each life needed is the threshold, read out directly:
The test asserts the doubling up to the limit and the plateau after it (within a factor of 1.5) and the retry count,
in all three configurations. If optimized code survives a collection (something else kept the object alive), the
attempt is discarded and repeated with a fresh function.
Testing
All on
main(cf1b36e) plus this commit; base = a shell built frommainitself.validateBytecode,validateGraphAtEachPhase,useConcurrentJIT=false,thresholdForJITAfterWarmUp=100,scribbleFreeCells) / eager thresholds(
thresholdForJITAfterWarmUp=10,thresholdForOptimizeAfterWarmUp=20) /useEagerCodeBlockJettisonTiming/collectContinuously: 34 to 36 failures per mode, the same files as the base shell in every mode (ICU/date,ensure-crash, eval-func-decl, ...), apart from two tests that flip on a loaded machine in both shells(
int8-repeat-in-then-out-of-bounds.js, written for a synchronous compiler, failed indefaulthere and incollectContinuouslyon the base;class-fields-to-property-key-const-string-ftl.jsfailed once on the base only), and the newtest, which the base shell fails (it does not know the option).
spread / iterator / for-of / super / closure / scope / varargs / apply / module / function / tdz / jettison / weak / reoptimiz,
x default / eager jettison timing / eager thresholds / no concurrent JIT /
collectContinuously): 17 to 18 failures per mode(+ 3 timeouts under
collectContinuously), the same files as the base ASAN shell exceptintl-datetimeformat-language-change-mid-construction.js(a worker-timing test; failed once here, passes 3 of 3 alone on both shells) and
activation-sink-osrexit-default-value-tdz-error.jsunder
collectContinuously(at the 20-minute limit on both; the base run finished it just under, it is on the base's timeout list in an earlier run).jettison timing,
collectContinuously,useFTLJIT=0,useJIT=0(exits early). One combination fails: limit 100 witheager thresholds, where a threshold of two calls does not double cleanly nine times; the run lines never combine them.
three limits): same sequence as the shell (28, 55, 109, 201, 201, ... calls per life at the default limit).