FTL: store OSRExitDescriptor values as a compact byte stream - #349
FTL: store OSRExitDescriptor values as a compact byte stream#349robobun wants to merge 1 commit into
Conversation
Each FTL OSRExitDescriptor held a dense FixedOperands<ExitValue> with one 9-byte ExitValue per frame operand (arguments + locals + tmps of the whole inlined frame). Most of those entries are ExitValueDead and the rest are small (a stack slot, a stackmap argument index, a constant), so a large inlined function with many exits multiplies a mostly-dead table across hundreds of descriptors. Replace m_values with a compact byte encoding held in a FixedVector<uint8_t>: a tag byte per entry carrying the ExitValueKind in the low four bits and a short Dead run length in the high four (overflow continues as a varint), a zig-zag varint virtual register for InJSStack* kinds, a DataFormat byte plus varint argument index for ExitValueArgument, eight raw bytes for ExitValueConstant, and a varint index into a small FixedVector<ExitTimeObjectMaterialization*> side table for ExitValueMaterializeNewObject. The descriptor keeps the operand counts so size()/numberOfLocals()/tmpIndex() remain derivable, and decodeValues() reconstructs the dense Operands<ExitValue> on demand. buildExitArguments now fills a local dense array and calls setValues() once; compileStub decodes once at the top and uses that copy unchanged; validateReferences and the dump paths decode on demand. The FTLCompile.cpp loop that eagerly rewrote every value with withLocalsOffset(localsOffset) becomes a stored m_localsOffset applied during decode (InJSStack* values whose virtual register is a local), which is exactly what ExitValue::withLocalsOffset did. ExitTimeObjectMaterialization::accountForLocalsOffset() is still applied eagerly. A debug-only round-trip check in setValues() asserts encode/decode equivalence for every kind.
WalkthroughChangesOSR exit value encoding
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/JavaScriptCore/ftl/FTLOSRExit.cpp`:
- Around line 138-142: Update the ExitValueMaterializeNewObject handling to
deduplicate objectMaterialization() pointers before appending to
materializations: reuse the existing index for a previously seen pointer and
append/store a new index only for unique pointers. Add or use a pointer-to-index
map in the surrounding encoding routine while preserving the encoded index
semantics for all materialization references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21f23dfc-b587-445f-bab8-1ebb8393b73c
📒 Files selected for processing (5)
Source/JavaScriptCore/ftl/FTLCompile.cppSource/JavaScriptCore/ftl/FTLLowerDFGToB3.cppSource/JavaScriptCore/ftl/FTLOSRExit.cppSource/JavaScriptCore/ftl/FTLOSRExit.hSource/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp
| case ExitValueMaterializeNewObject: | ||
| bytes.append(static_cast<uint8_t>(value.kind())); | ||
| encodeVarint(bytes, materializations.size()); | ||
| materializations.append(value.objectMaterialization()); | ||
| break; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Dedup repeated materialization pointers instead of storing them once per occurrence.
Each time an ExitValueMaterializeNewObject operand is encountered, its pointer is unconditionally appended to materializations, so a materialized object referenced by several locals/tmps/heap slots gets stored redundantly (8 bytes per occurrence) rather than once. Since deduplicating shared materializations is exactly the kind of storage this PR is trying to cut, mapping each unique pointer to a single index would avoid this.
♻️ Proposed dedup via index map
Vector<uint8_t> bytes;
Vector<ExitTimeObjectMaterialization*> materializations;
+ UncheckedKeyHashMap<ExitTimeObjectMaterialization*, unsigned> materializationIndices; case ExitValueMaterializeNewObject:
bytes.append(static_cast<uint8_t>(value.kind()));
- encodeVarint(bytes, materializations.size());
- materializations.append(value.objectMaterialization());
+ encodeVarint(bytes, materializationIndices.ensure(value.objectMaterialization(), [&] {
+ materializations.append(value.objectMaterialization());
+ return materializations.size() - 1;
+ }).iterator->value);
break;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case ExitValueMaterializeNewObject: | |
| bytes.append(static_cast<uint8_t>(value.kind())); | |
| encodeVarint(bytes, materializations.size()); | |
| materializations.append(value.objectMaterialization()); | |
| break; | |
| Vector<uint8_t> bytes; | |
| Vector<ExitTimeObjectMaterialization*> materializations; | |
| UncheckedKeyHashMap<ExitTimeObjectMaterialization*, unsigned> materializationIndices; | |
| case ExitValueMaterializeNewObject: | |
| bytes.append(static_cast<uint8_t>(value.kind())); | |
| encodeVarint(bytes, materializationIndices.ensure(value.objectMaterialization(), [&] { | |
| materializations.append(value.objectMaterialization()); | |
| return materializations.size() - 1; | |
| }).iterator->value); | |
| break; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/JavaScriptCore/ftl/FTLOSRExit.cpp` around lines 138 - 142, Update the
ExitValueMaterializeNewObject handling to deduplicate objectMaterialization()
pointers before appending to materializations: reuse the existing index for a
previously seen pointer and append/store a new index only for unique pointers.
Add or use a pointer-to-index map in the surrounding encoding routine while
preserving the encoded index semantics for all materialization references.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this is a ~250-line change to FTL OSR exit descriptor storage — a correctness-critical path where encoding mistakes silently miscompile JS — so it deserves a human pass.
What was reviewed:
- Encode/decode symmetry for all
ExitValueKindcases, dead-run length encoding (short + LEB overflow), and zigzag varint for virtual registers. - Lazy
m_localsOffsetapplication indecodeValues()matchesExitValue::withLocalsOffsetexactly (onlyInJSStack*withisLocal()). - All former
m_valuesconsumers (compileStub,validateReferences, dump paths,FTLCompile) now go throughdecodeValues()/setLocalsOffset(); no stale references remain. ExitValueKindmax value (8) fits the 4-bit tag;ExitArgument::argument()isunsignedso the uint32 varint is lossless.
Extended reasoning...
Overview
This PR replaces OSRExitDescriptor::m_values (a dense FixedOperands<ExitValue>) with a compact byte-stream encoding: a FixedVector<uint8_t> holding tag-prefixed variable-length records plus a side table of materialization pointers. Producers (LowerDFGToB3::buildExitArguments) encode once via setValues(); consumers (compileStub, validateReferences, verbose dumps) decode on demand via decodeValues(). The eager per-value withLocalsOffset rewrite in FTLCompile.cpp becomes a stored m_localsOffset applied lazily during decode. Five files touched, all under ftl/.
Security risks
None in the traditional sense — no untrusted input reaches the encoder/decoder; the byte stream is self-produced compiler metadata. The real risk is correctness: an encode/decode mismatch would corrupt the operand map used to reconstruct baseline frame state on OSR exit, producing wrong values in locals/tmps after deoptimization. That's a silent-miscompilation class of bug, not an exploit surface, but it's the highest-impact failure mode in a JIT.
Level of scrutiny
High. FTL OSR exit is on the critical path for correctness of all optimized code — every speculation failure flows through compileStub, which now depends on decodeValues() reproducing exactly what setValues() was given (modulo the lazy locals-offset). The change is well-structured and the debug round-trip assertion is strong evidence, but ~250 lines of new hand-rolled binary encoding in this layer warrants a human reviewer who knows the FTL exit machinery signing off.
Other factors
Points that raised my confidence: the static_assert on ExitValueMaterializeNewObject <= mask guards the 4-bit tag; zigzag encode/decode are the standard formulas; the dead-run scheme correctly handles the run == maxShortDeadRun boundary (emits varint 0); decodeValues() RELEASE_ASSERTs both index == size and offset == bytes.size() so over/under-consumption crashes rather than silently corrupts; I confirmed ExitValue::withLocalsOffset only touches InJSStack* locals, matching the lazy path; and grep shows no remaining m_values references in ftl/. The PR description reports identical stress-test failure sets vs baseline under debug+ASAN with the round-trip assertion active. Still, given the blast radius, I'm deferring rather than approving.
Preview Builds
|
2 participants
Third instalment of the optimizing-tier metadata compaction, following #334 (DFG
VariableEventStream) and #335 (DFGOSREntryData::m_expectedValues).What
Each FTL
OSRExitDescriptorcurrently holds a denseFixedOperands<ExitValue>: one 9-byteExitValueper frame operand (arguments + locals + tmps of the whole inlined frame). Most entries areExitValueDeadand the remainder are small (a virtual register, a stackmap argument index, a constant, or a materialization pointer), so a large inlined function with many exits multiplies a mostly-dead table across hundreds of descriptors. On JS-heavy workloads this was the single largest piece of resident FTL metadata.This replaces
m_valueswith a compact byte stream held in aFixedVector<uint8_t>plus a smallFixedVector<ExitTimeObjectMaterialization*>side table:ExitValueKindin the low 4 bits, a short Dead run length in the high 4 bits (overflow continues as an unsigned LEB varint);ExitValueInJSStack*: zig-zag varint of the virtual-register offset;ExitValueArgument: oneDataFormatbyte + varint argument index;ExitValueConstant: 8 rawEncodedJSValuebytes;ExitValueMaterializeNewObject: varint index into the side table.The descriptor keeps
m_numberOfArguments/m_numberOfLocals/m_numberOfTmpssosize()/numberOfLocals()/tmpIndex()remain derivable.decodeValues()reconstructs the denseOperands<ExitValue>on demand.Producers and consumers
LowerDFGToB3::buildExitArgumentsfills a localOperands<ExitValue>and callssetValues()once.FTLOSRExitCompiler::compileStubdecodes once at the top and uses the dense copy exactly as before, so exit-thunk codegen is untouched.OSRExitDescriptor::validateReferencesand theverboseOSR/FINALIZE_CODE_IFdump paths decode on demand.FTLCompile.cppno longer rewrites every value withwithLocalsOffset(localsOffset); insteadsetLocalsOffset(localsOffset)stores the offset anddecodeValues()applies it lazily toInJSStack*values whose virtual register is a local, which is exactly whatExitValue::withLocalsOffsetdid.ExitTimeObjectMaterialization::accountForLocalsOffset()is still applied eagerly in the same loop.A debug-only round-trip check in
setValues()asserts encode/decode equivalence for everyExitValueKind.USE(JSVALUE32_64)builds are unaffected: all of this is inside#if ENABLE(FTL_JIT), whichPlatformEnable.hforces off on 32-bit.Byte totals
Process-lifetime totals for
OSRExitDescriptorvalue storage, counted atsetValues()time (release x64jsc, 4 runs each):FixedOperands<ExitValue>--tag=octane--tag=typescriptRoughly an 11× reduction. JetStream3 Octane score is within noise of baseline on a non-isolated host (baseline 212-338 pts, patched 304-328 pts over 4 runs each).
Tests
368 FTL/OSR-focused
JSTests/stressfiles (ftl-*,*osr*,*material*,*sink*,*checkpoint*,*phantom*) under two configurations (eager tier-up:--useConcurrentJIT=0 --thresholdForOptimizeAfterWarmUp=10 --thresholdForFTLOptimizeAfterWarmUp=20, and default thresholds):Failure sets are identical modulo one timeout/flake each way; the one release discrepancy (
check-is-constant-non-cell-should-not-array-profile-during-osr-exit.jsunder the eager config) reproduces 5/5 on both binaries when re-run in isolation. The round-trip assertion held across every descriptor encoded in the debug runs.