Cache source constructor name across prototype transitions - #312
Conversation
|
Warning Review limit reached
Next review available in: 1 minute 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 (3)
WalkthroughChangesThe change adds conditional source constructor name storage and lookup to Constructor name tracking
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
24c318e to
abf22f8
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (protoObject == globalObject->objectPrototype()) | ||
| return String(); |
There was a problem hiding this comment.
🟡 Only objectPrototype() is skipped here, but class X extends Y also reaches changePrototypeTransition — with a JSFunction whose stored prototype is Function.prototype. The lookup finds Function.prototype.constructor (an InternalFunction), so every subclass declaration now allocates a StructureRareData on its ChangePrototype transition just to cache the string "Function". Consider extending the early-out to globalObject->functionPrototype() so class-extends stays rare-data-free, matching the intent behind the Object.prototype skip.
Extended reasoning...
What happens
class X extends Y emits emitDirectSetPrototypeOf(constructor, superclass) (NodesCodegen.cpp:5743), which flows through globalFuncSetPrototypeDirectOrThrow → JSObject::setPrototypeDirect → Structure::changePrototypeTransition. The freshly-created class constructor is a JSFunction whose structure's storedPrototype() is Function.prototype (all the function structures in JSGlobalObject.cpp are built with m_functionPrototype).
In computeSourceConstructorName:
protoObjectisFunction.prototype, which is notglobalObject->objectPrototype(), so the early-out on line 756 does not fire.Function.prototypehas an own value propertyconstructorset toFunctionConstructor(JSGlobalObject.cpp:m_functionPrototype->putDirectWithoutTransition(vm, vm.propertyNames->constructor, functionConstructor, ...)).FunctionConstructorinherits fromInternalFunction, sojsDynamicCast<InternalFunction*>succeeds andfunction->name()returns"Function".
Back in changePrototypeTransition, sourceName is non-empty, so transition->ensureRareData(vm)->setSourceConstructorName(...) runs. Previously these ChangePrototype transition structures needed no rare data (setMaxOffset stays in the short range for functions), so this is a new StructureRareData allocation per unique (base-function-structure, superclass) pair — plus a Function.prototype.constructor property lookup on every class-extends evaluation, since computeSourceConstructorName runs before the transition-table cache is checked.
Step-by-step example
class Base {}
class Sub extends Base {}- Codegen for
SubcallssetPrototypeDirect(SubConstructor, Base). changePrototypeTransitioncallscomputeSourceConstructorName(vm, SubConstructor->structure()).SubConstructor->structure()->storedPrototype()==Function.prototype≠Object.prototype→ no early return.getOwnPropertySlot(Function.prototype, "constructor")→FunctionConstructor(value slot).jsDynamicCast<InternalFunction*>succeeds → returns"Function".- New transition structure calls
ensureRareData(vm)and stores"Function".
Why it isn't useful
The cached "Function" string is technically correct but never needed: a class constructor is still a JSFunction after reparenting, and util.inspect/calculatedClassName already derive its display from the function's own name / class info. The PR description explicitly special-cases Object.prototype "so Object.create(null) allocates no rare data" — the same reasoning applies to Function.prototype on the class-extends path, which is at least as hot.
Suggested fix
Extend the early-out to also skip Function.prototype:
if (protoObject == globalObject->objectPrototype()
|| protoObject == globalObject->functionPrototype())
return String();You might also consider skipping other well-known built-in prototypes (Array.prototype, etc.) for the same reason, though Function.prototype is the one on a hot bytecode-driven path.
Impact
Memory/CPU overhead only — nothing breaks functionally. One extra StructureRareData per subclass declaration and one extra property lookup per class-extends evaluation. Marking as a nit.
| #if USE(BUN_JSC_ADDITIONS) | ||
| { | ||
| String sourceName = structure->sourceConstructorName(); | ||
| if (!sourceName.isEmpty()) | ||
| transition->ensureRareData(vm)->setSourceConstructorName(WTF::move(sourceName)); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
🟡 The forward-copy here handles toDictionaryTransition, but two other paths also pin() a fresh transition (clearing previousID) without copying the cached name: nonPropertyTransitionSlow's isDictionary() branch and setBrandTransition's isDictionary() branch. So setPrototypeOf(new Foo(), null) → force dictionary → Object.preventExtensions() (or seal/freeze/private-brand) drops the name and util.inspect falls back to Object. Minor edge case — worth adding the same forward-copy in those two branches for completeness.
Extended reasoning...
What the bug is
Structure::pin() (StructureInlines.h:483) calls clearPreviousID(), so any structure that has been pinned can no longer walk back through previousID() to find a cached sourceConstructorName. This PR handles that for changePrototypeTransition (stores the name directly on the new structure's rare data) and for toDictionaryTransition (copies the name forward before pinning). But there are two more transition paths that call pin() on a freshly-created transition without copying the name forward:
nonPropertyTransitionSlow, in theif (structure->isDictionary())branch near the end. This is reached byObject.seal,Object.freeze,Object.preventExtensions,becomePrototypeTransition, and indexing-type changes when the source structure is already a dictionary.setBrandTransition, in theif (structure->isDictionary())branch.
In both cases the new transition is created via Structure::create(vm, structure, ...) (which sets previousID to the source), then immediately pinned, which clears previousID. The new structure's rare data has never had setSourceConstructorName called on it, so sourceConstructorName() on the resulting structure walks previousID() → nullptr immediately and returns an empty string.
Step-by-step example
class Foo {}
const o = new Foo();
Object.setPrototypeOf(o, null); // changePrototypeTransition: caches "Foo" on rare data of the new structure
for (let i = 0; i < 200; i++) o['k'+i] = i; // eventually toDictionaryTransition: PR copies "Foo" forward — OK
Object.preventExtensions(o); // nonPropertyTransitionSlow, source isDictionary → transition->pin()
// → clearPreviousID(), name NOT copied
util.inspect(o); // sourceConstructorName() returns "", falls back to "Object"Walking through nonPropertyTransitionSlow for the last step:
structure->isDictionary()is true (it became a dictionary in the previous step), so the existing-transition lookup is skipped.transition = Structure::create(vm, structure, deferred)—previousIDinitially points at the dictionary structure that does carry the cached name.preventsExtensions(transitionKind)is true →setDidPreventExtensions(true). It is not seal/freeze, so it takes theelsebranch (takePropertyTableOrCloneIfPinned).- Then
if (structure->isDictionary()) { transition->pin(...); }runs →clearPreviousID(). - The new structure has no rare-data
m_sourceConstructorNameand nopreviousID, sosourceConstructorName()returnsString().
The same reasoning applies to setBrandTransition when a private brand is stamped onto an object that is already a dictionary.
Why existing code doesn't prevent it
sourceConstructorName() relies on either (a) the current structure's rare data holding the name, or (b) previousID() being intact so it can walk back to the structure that does. pin() breaks (b), and only changePrototypeTransition / toDictionaryTransition establish (a). The non-dictionary paths in nonPropertyTransitionSlow are fine — pinForCaching does not clear previousID, and the transition-table path preserves it — so this only bites when the source is already a dictionary.
Impact
Cosmetic only: util.inspect on such an object shows [Object: null prototype] { ... } instead of [Foo: null prototype] { ... }. Requires the fairly narrow sequence of setPrototypeOf → dictionary conversion → seal/freeze/preventExtensions/private-brand. No crash, no JS-semantic change. The PR still improves the common case; this is a completeness gap in the caching.
Fix
Add the same forward-copy block used in toDictionaryTransition to the two isDictionary() pin sites (or, more robustly, hoist it into a helper invoked wherever a fresh transition is about to be pin()ed):
#if USE(BUN_JSC_ADDITIONS)
{
String sourceName = structure->sourceConstructorName();
if (!sourceName.isEmpty())
transition->ensureRareData(vm)->setSourceConstructorName(WTF::move(sourceName));
}
#endif| #if USE(BUN_JSC_ADDITIONS) | ||
| String sourceName = computeSourceConstructorName(vm, structure); | ||
| #endif |
There was a problem hiding this comment.
🟡 Nit: computeSourceConstructorName() runs before the shouldChain / m_transitionTable.get() early return, so on repeated Object.setPrototypeOf(new Foo(), null) with same-shaped objects the previousID() walk + VMInquiry constructor lookup happens every time and the result is discarded. Since the transition-table probe doesn't allocate, you could hoist it above both this call and DeferGC and skip the compute on cache hits — minor, but it keeps the fast path as cheap as it was before.
Extended reasoning...
What the issue is
changePrototypeTransition now calls computeSourceConstructorName(vm, structure) unconditionally at the top of the function, before the cached-transition fast path:
#if USE(BUN_JSC_ADDITIONS)
String sourceName = computeSourceConstructorName(vm, structure);
#endif
DeferGC deferGC(vm);
...
if (shouldChain) {
if (Structure* existingTransition = structure->m_transitionTable.get(key, 0, TransitionKind::ChangePrototype)) {
...
return existingTransition; // sourceName is discarded
}
}When an existing ChangePrototype transition is already in the table, the function returns it immediately and sourceName is thrown away — but the work to compute it has already been done.
Concrete walk-through
Consider a loop like:
class Foo {}
for (let i = 0; i < N; i++) Object.setPrototypeOf(new Foo(), null);- First iteration:
new Foo()produces an object with structure S (prototypeFoo.prototype).computeSourceConstructorNamewalksS'spreviousID()chain (no cached name found), then does a VMInquirygetOwnPropertySlotfor"constructor"onFoo.prototype, reads the function name"Foo". No cached transition exists yet, so a new structure T is created,"Foo"is stored on T's rare data, and T is inserted into S's transition table. - Second iteration: Another
new Foo()again has structure S.computeSourceConstructorNameis called again on S. The cached name lives on T's rare data, not on S or its ancestors, sosourceConstructorName()finds nothing and falls through to thegetOwnPropertySlotVMInquiry onFoo.prototypeagain. ThenshouldChainis true,m_transitionTable.get(nullptr, 0, ChangePrototype)returns T, and the function returns early — the freshly computed"Foo"is discarded. - Steps in (2) repeat for every subsequent iteration.
So the transition-table caching that previously made repeated same-shape setPrototypeOf a simple hash-table hit now also pays for a previousID() walk plus a VMInquiry own-property lookup on every call.
Why nothing prevents it
The compute is deliberately placed before DeferGC (per the PR description, since the property lookup could allocate). But the transition-table probe itself (StructureTransitionTable::get, whether single-slot or map) doesn't allocate — it's a pointer compare or a read-only hash lookup. So the ordering constraint against DeferGC doesn't force the compute to precede the cache check.
Impact
This is a minor performance nit, not a correctness issue. setPrototypeOf is already an inherently slow, deoptimizing operation; the extra cost is a short pointer-chain walk plus one VMInquiry own-property lookup (no user JS runs). The Object.prototype → null case (Object.create(null)) already short-circuits before the lookup. It's unlikely to be measurable in real workloads, but it does make the cached-transition path strictly more expensive than it was before this PR.
Suggested fix
Duplicate the fast-path probe ahead of the compute (and ahead of DeferGC), roughly:
JSObject* key = prototype.isNull() ? nullptr : asObject(prototype);
bool shouldChain = !structure->hasPolyProto() && structure->typeInfo().type() != GlobalObjectType && !structure->hasBeenDictionary();
if (shouldChain) {
if (Structure* existingTransition = structure->m_transitionTable.get(key, 0, TransitionKind::ChangePrototype))
return existingTransition;
}
#if USE(BUN_JSC_ADDITIONS)
String sourceName = computeSourceConstructorName(vm, structure);
#endif
DeferGC deferGC(vm);
...(or equivalently, keep a single shouldChain block but move both it and the compute above DeferGC). Either way the redundant lookup on the cache-hit path goes away.
abf22f8 to
0bab8a9
Compare
Preview Builds
|
0bab8a9 to
73c011c
Compare
The first preview build was based on oven-sh/WebKit main which includes the upstream 2603e9eb41f0 merge (DeferredWorkTimer::TicketData removal) that bun's main does not have yet. Rebased oven-sh/WebKit#312 onto 639550acdcb2 (bun's current pin) so the preview prebuilt is ABI-compatible. jsFunctionGetSourceConstructorName now returns undefined for non FinalObjectType instances. V8's GetConstructorName() reports the base type for Error/Array/etc. subclasses after setPrototypeOf(null), which inspect.js already recovers via Object.prototype.toString; only plain objects need the Structure cache. Added a 30s per-test timeout to the 'no assertion failures 2' block, which runs ~1500 assertions and exceeds the 5s default under the debug+ASAN build regardless of this change.
|
Rebased onto 639550a (bun's current WEBKIT_VERSION) so the preview prebuilt is ABI-compatible with oven-sh/bun main. The previous preview was based on current main which includes the upstream 2603e9e merge and breaks bun's build until oven-sh/bun#34373 lands. Consumed in oven-sh/bun#34755. |
73c011c to
0bab8a9
Compare
…nces
Object.setPrototypeOf(new Foo(), null) now inspects as
'[Foo: null prototype] {}' instead of '[Object: null prototype] {}',
matching Node.js.
Node recovers the name via V8's Map::constructor back-reference which
survives prototype transitions. JSC's changePrototypeTransition pins the
new Structure and clears previousID(), so neither the old prototype nor
its constructor is reachable from the object afterwards.
The JSC side (oven-sh/WebKit#312, preview autobuild-preview-pr-312-0bab8a9a)
caches the previous prototype's constructor name on StructureRareData
during changePrototypeTransition and carries it through
toDictionaryTransition. Structure::sourceConstructorName() walks
previousID() to find it. The cache is a plain WTF::String so it adds no
GC root.
On the Bun side, internalGetConstructorName consults a new
jsFunctionGetSourceConstructorName binding before falling back to
'[object X]' parsing. The binding only answers for FinalObjectType
instances; V8 reports the base type for Error/Array/etc. subclasses
after setPrototypeOf(null), which Object.prototype.toString already
covers.
The 'no assertion failures 2' test block (~1500 util.inspect assertions)
already exceeds the 5s default under debug+ASAN on main; gave it a 30s
ceiling so the file is runnable via bun bd.
0bab8a9 to
cd20f60
Compare
When setPrototypeOf() clears a Structure's previousID (via pin()), the link back to the original prototype's constructor is lost. Node's util.inspect recovers the class name for null-prototype objects via V8's Map::constructor back-reference, producing '[Foo: null prototype]' for Object.setPrototypeOf(new Foo(), null). Cache the name on StructureRareData at the point of transition: changePrototypeTransition computes it from the previous prototype's own 'constructor' property (VMInquiry, no JS execution) or from a previously cached value, and toDictionaryTransition carries it forward. The lookup runs before DeferGC; the cache is a plain WTF::String so it adds no GC root. Used by Bun to implement Node-compatible util.inspect output for null-prototype class instances.
cd20f60 to
d4aabd1
Compare
…nces
Object.setPrototypeOf(new Foo(), null) now inspects as
'[Foo: null prototype] {}' instead of '[Object: null prototype] {}',
matching Node.js.
Node recovers the name via V8's Map::constructor back-reference which
survives prototype transitions. JSC's changePrototypeTransition pins the
new Structure and clears previousID(), so neither the old prototype nor
its constructor is reachable from the object afterwards.
The JSC side (oven-sh/WebKit#312, preview autobuild-preview-pr-312-0bab8a9a)
caches the previous prototype's constructor name on StructureRareData
during changePrototypeTransition and carries it through
toDictionaryTransition. Structure::sourceConstructorName() walks
previousID() to find it. The cache is a plain WTF::String so it adds no
GC root.
On the Bun side, internalGetConstructorName consults a new
jsFunctionGetSourceConstructorName binding before falling back to
'[object X]' parsing. The binding only answers for FinalObjectType
instances; V8 reports the base type for Error/Array/etc. subclasses
after setPrototypeOf(null), which Object.prototype.toString already
covers.
The 'no assertion failures 2' test block (~1500 util.inspect assertions)
already exceeds the 5s default under debug+ASAN on main; gave it a 30s
ceiling so the file is runnable via bun bd.
|
Reworked |
| #if USE(BUN_JSC_ADDITIONS) | ||
| { | ||
| String sourceName = structure->sourceConstructorName(); | ||
| if (!sourceName.isEmpty()) | ||
| transition->ensureRareData(vm)->setSourceConstructorName(WTF::move(sourceName)); | ||
| } |
There was a problem hiding this comment.
🟡 Nit: sourceConstructorName() here walks the full previousID() chain (~128–512 hops at the dictionary-transition threshold) for every object that becomes a dictionary, even though the vast majority never went through changePrototypeTransition and have no cached name anywhere in their ancestry — the walk always reaches the root and returns empty. A sticky bit in m_bitField propagated through the Structure(VM&, StructureVariant, Structure* previous) copy-ctor (like hasBeenDictionary/mayBePrototype) would let the common case skip the walk in O(1). Very minor — the transition already does O(N) work cloning the property table — so feel free to defer.
Extended reasoning...
What this adds
toDictionaryTransition now calls structure->sourceConstructorName() unconditionally before pinning:
String sourceName = structure->sourceConstructorName();
if (!sourceName.isEmpty())
transition->ensureRareData(vm)->setSourceConstructorName(WTF::move(sourceName));sourceConstructorName() (Structure.cpp:726–735) is a plain previousID() walk to the root, checking each structure's rare data for a cached name. Dictionary conversion is triggered by shouldDoCacheableDictionaryTransitionForAdd() exactly when transitionCountEstimate() exceeds s_maxTransitionLength (128) or s_maxTransitionLengthForNonEvalPutById (512), so the chain being walked is on the order of hundreds of Structure* dereferences — each of which loads m_previousOrRareData and does an isRareData type check on the pointed-to cell.
For the overwhelmingly common case — an object that grew many properties but never had setPrototypeOf called on it — no ancestor carries a cached name, so the walk always reaches the root and returns String(). The ensureRareData/setSourceConstructorName that follows is correctly skipped, but the pointer-chase itself is pure overhead.
Step-by-step example
const o = {};
for (let i = 0; i < 600; i++) o['k' + i] = i;At roughly the 512th putById, addNewPropertyTransition sees transitionCountEstimate() > 512 and calls toCacheableDictionaryTransition. The source structure's previousID() chain is ~512 links of plain PropertyAddition transitions rooted at the empty-object structure. sourceConstructorName() dereferences all ~512 m_previousOrRareData pointers (scattered Structure cells, each with a header load for isRareData), finds nothing, and returns empty. Before this PR, the same transition did no chain walk in the common case — addNewPropertyTransition hands the property table forward via takePropertyTableOrCloneIfPinned, so copyPropertyTableForPinning here just clones the existing table without touching previousID().
Why nothing prevents it
The forward-copy is guarded only by !sourceName.isEmpty() after the walk. There's no cheap flag on Structure indicating "some ancestor holds a cached source-constructor name" that would let the common case bail out without walking.
Impact / why this is only a nit
toDictionaryTransition is one-time per object (at most twice: cacheable → uncacheable) and already does O(N) work: copyPropertyTableForPinning clones an ~N-entry PropertyTable, and if GC has cleared m_propertyTableUnsafe it falls into materializePropertyTable which itself walks previousID(). So the added cost is a constant-factor increase on an already-O(N) one-shot path, not a new asymptotic behavior — likely unmeasurable in practice. The distinction is only that the property-table clone is necessary contiguous-memory work, whereas this is an unnecessary scattered-pointer chase for the >99% of dictionary conversions that never involved changePrototypeTransition.
Suggested fix
Add a sticky bit alongside the existing ones in m_bitField — e.g. DEFINE_BITFIELD(bool, hasCachedSourceConstructorName, HasCachedSourceConstructorName, 1, 31) — set it in changePrototypeTransition when setSourceConstructorName runs, and copy it forward in the Structure(VM&, StructureVariant, Structure* previous) constructor the same way hasBeenDictionary/mayBePrototype are copied. Then both this call site and the one at the top of computeSourceConstructorName can guard the walk with a single-bit test, making the common case O(1).
(Distinct from the other comment at this location, which is about the name being lost on other pin() paths; this is about the walk being unconditional on a path that almost never has anything to find.)
1 participant
When
setPrototypeOf()clears a Structure'spreviousID(viapin()), the link back to the original prototype's constructor is lost. Node'sutil.inspectrecovers the class name for null-prototype objects via V8'sMap::constructorback-reference, producing[Foo: null prototype]forObject.setPrototypeOf(new Foo(), null). JSC has no equivalent:calculatedClassName()falls back to"Object"once the prototype is gone.This caches the name on
StructureRareDataat the point of transition:changePrototypeTransitioncomputes it from the previous prototype's ownconstructorproperty (VMInquiry only, no JS execution) or from a previously cached value, and stores the result on the new structure's rare data. The lookup runs beforeDeferGC. The commonObject.prototype→nullpath is skipped soObject.create(null)allocates no rare data.toDictionaryTransitioncopies an existing cached name forward so dictionary conversion aftersetPrototypeOfdoes not lose it.Structure::sourceConstructorName()walkspreviousID()to the first cached value so property-add transitions after the prototype change can still find it.The cache is a plain
WTF::String, so it adds no GC root (the old prototype can still be collected). All additions are guarded byUSE(BUN_JSC_ADDITIONS).Consumed on the Bun side by
internalGetConstructorNameinutil.inspectto match Node's output for null-prototype class instances.