Skip to content

Commit

Permalink
[JSC] AccessCase should not hold CallLinkInfo*
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=268221
rdar://121733122

Reviewed by Justin Michaud.

AccessCase holds CallLinkInfo*. But when the underlying JITStubRoutine gets destroyed, this becomes invalid.
Previously, it does not matter since we always destroy CodeBlock first (synchronously), and then we clean up JITStubRoutine.
So there were strict ordering.  But now CodeBlock destruction can get delayed.

But fundamentally speaking, having CallLinkInfo* in AccessCase is not right. This is compiled code's data structure and
AccessCase should be just a data for IC feedback.

In this patch we decouple CallLinkInfo* from AccessCase. CallLinkInfo's lifetime should be correctly managed by visitWeak, so,
we add visitWeak iteration in MarkingGCAwareJITStubRoutine. Then we can remove CallLinkInfo from AccessCase.

* JSTests/stress/decouple-calllinkinfo-from-access-case.js: Added.
(F7):
(f25):
(f33):
(C20.prototype.valueOf):
(C20):
(f27):
* Source/JavaScriptCore/bytecode/AccessCase.cpp:
(JSC::AccessCase::forEachDependentCell const):
(JSC::AccessCase::doesCalls const):
(JSC::AccessCase::visitWeak const):
(JSC::AccessCase::collectDependentCells const):
* Source/JavaScriptCore/bytecode/AccessCase.h:
* Source/JavaScriptCore/bytecode/GetByStatus.cpp:
(JSC::GetByStatus::computeForStubInfoWithoutExitSiteFeedback):
* Source/JavaScriptCore/bytecode/GetterSetterAccessCase.cpp:
(JSC::GetterSetterAccessCase::dumpImpl const):
* Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:
* Source/JavaScriptCore/bytecode/InlineCacheCompiler.cpp:
(JSC::InlineCacheCompiler::generateWithGuard):
(JSC::InlineCacheCompiler::generate):
(JSC::InlineCacheCompiler::generateImpl):
(JSC::InlineCacheCompiler::emitProxyObjectAccess):
(JSC::InlineCacheCompiler::regenerate):
(JSC::InlineCacheHandler::callLinkInfoAt):
(JSC::InlineCacheHandler::visitWeak const):
* Source/JavaScriptCore/bytecode/InlineCacheCompiler.h:
* Source/JavaScriptCore/bytecode/ProxyObjectAccessCase.cpp:
(JSC::ProxyObjectAccessCase::dumpImpl const):
* Source/JavaScriptCore/bytecode/ProxyObjectAccessCase.h:
* Source/JavaScriptCore/bytecode/PutByStatus.cpp:
(JSC::PutByStatus::computeForStubInfo):
* Source/JavaScriptCore/bytecode/StructureStubInfo.cpp:
(JSC::StructureStubInfo::callLinkInfoAt):
* Source/JavaScriptCore/bytecode/StructureStubInfo.h:
* Source/JavaScriptCore/jit/GCAwareJITStubRoutine.cpp:
(JSC::MarkingGCAwareJITStubRoutine::MarkingGCAwareJITStubRoutine):
(JSC::MarkingGCAwareJITStubRoutine::visitWeakImpl):
(JSC::MarkingGCAwareJITStubRoutine::callLinkInfoAtImpl):
(JSC::GCAwareJITStubRoutineWithExceptionHandler::GCAwareJITStubRoutineWithExceptionHandler):
(JSC::createICJITStubRoutine):
* Source/JavaScriptCore/jit/GCAwareJITStubRoutine.h:
* Source/JavaScriptCore/jit/JITStubRoutine.cpp:
(JSC::JITStubRoutine::callLinkInfoAt):
* Source/JavaScriptCore/jit/JITStubRoutine.h:
(JSC::JITStubRoutine::callLinkInfoAtImpl):

Originally-landed-as: 272448.633@safari-7618-branch (f25738c). rdar://128077399
Canonical link: https://commits.webkit.org/278779@main
  • Loading branch information
Constellation committed May 14, 2024
1 parent 6ad266d commit b251507
Show file tree
Hide file tree
Showing 17 changed files with 210 additions and 204 deletions.
96 changes: 96 additions & 0 deletions JSTests/stress/decouple-calllinkinfo-from-access-case.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
// runDefault("--validateOptions=true", "--thresholdForJITSoon=10", "--thresholdForJITAfterWarmUp=10", "--thresholdForOptimizeAfterWarmUp=100", "--thresholdForOptimizeAfterLongWarmUp=100", "--thresholdForOptimizeSoon=100", "--thresholdForFTLOptimizeAfterWarmUp=1000", "--thresholdForFTLOptimizeSoon=1000", "--validateBCE=true")

const ProxyConstructor = Proxy;
const getPrototypeOf = Object.getPrototypeOf;
const ReflectGet = Reflect.get;
const ReflectSet = Reflect.set;
const ReflectHas = Reflect.has;
const setPrototypeOf = Object.setPrototypeOf;

function probe(id, value) {
let originalPrototype, newPrototype;
let handler = {
get(target, key, receiver) {
if (key === '__proto__' && receiver === value) return originalPrototype;
if (receiver === newPrototype) return ReflectGet(target, key);
return ReflectGet(target, key, receiver);
},
set(target, key, value, receiver) {
if (receiver === newPrototype) return ReflectSet(target, key, value);
return ReflectSet(target, key, value, receiver);
},
has(target, key) {
return ReflectHas(target, key);
},
};

try {
originalPrototype = getPrototypeOf(value);
newPrototype = new ProxyConstructor(originalPrototype, handler);
setPrototypeOf(value, newPrototype);
} catch (e) {}
}

probe("v1", "2003629588");
let v4 = 9150;
v4--;
probe("v6", 51828);
function F7(a9, a10, a11) {
if (!new.target) { throw 'must be called with new'; }
const v12 = this?.constructor;
try { new v12(this, "object", 447824390); } catch (e) {}
a11 % a11;
this.b = a9;
this.g = a10;
}
const v15 = new F7("2003629588", "object", 447824390);
const v16 = new F7(v15, v4, v4);
const v17 = new F7("2003629588", 51828, 51828);
probe("v17", v17);
const v18 = v17?.constructor;
probe("v18", v18);
let v19;
try { v19 = new v18("r", v17, "r"); } catch (e) {}
probe("v19", v19);
const v20 = [v17,v17];
probe("v20", v20);
const v21 = [F7,v15,v20,v15,v4];
const v22 = [v4,"object",51828];
probe("v22", v22);
let v23;
try { v23 = v22.reduce(v15); } catch (e) {}
const v24 = [2,-354747782,-16,10251,-1485280459,5,6,536870888,-47153,-193790246];
probe("v24", v24);
function f25(a26, a27) {
const o28 = {
[a27]: a26,
"d": v21,
};
return o28;
}
f25(v16, v22);
f25(v23, v16);
f25(v15, v22);
v24[4];
function f33(a34, a35, a36, a37) {
probe("v36", a36);
~a35;
v22.length = 1;
a36?.[v21];
}
v24.flatMap(f33);
gc();
class C20 {
valueOf(a22, a23) {
return ("n")[1204] - this;
}
}
const v26 = new C20();
function f27(a28, a29) {
new BigInt64Array(3603);
return v26 * v26;
}
try {
v26[Symbol.toPrimitive] = f27;
v26.valueOf();
} catch { }
146 changes: 8 additions & 138 deletions Source/JavaScriptCore/bytecode/AccessCase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -701,9 +701,6 @@ void AccessCase::forEachDependentCell(VM&, const Functor& functor) const
switch (type()) {
case Getter:
case Setter: {
auto& accessor = this->as<GetterSetterAccessCase>();
if (accessor.callLinkInfo())
accessor.callLinkInfo()->forEachDependentCell(functor);
break;
}
case CustomValueGetter:
Expand Down Expand Up @@ -731,9 +728,6 @@ void AccessCase::forEachDependentCell(VM&, const Functor& functor) const
case ProxyObjectLoad:
case ProxyObjectStore:
case IndexedProxyObjectLoad: {
auto& accessor = this->as<ProxyObjectAccessCase>();
if (accessor.callLinkInfo())
accessor.callLinkInfo()->forEachDependentCell(functor);
break;
}
case InstanceOfHit:
Expand Down Expand Up @@ -844,7 +838,7 @@ void AccessCase::forEachDependentCell(VM&, const Functor& functor) const
}
}

bool AccessCase::doesCalls(VM& vm, Vector<JSCell*>* cellsToMarkIfDoesCalls) const
bool AccessCase::doesCalls(VM&) const
{
bool doesCalls = false;
switch (type()) {
Expand Down Expand Up @@ -971,12 +965,6 @@ bool AccessCase::doesCalls(VM& vm, Vector<JSCell*>* cellsToMarkIfDoesCalls) cons
doesCalls = viaGlobalProxy();
break;
}

if (doesCalls && cellsToMarkIfDoesCalls) {
forEachDependentCell(vm, [&](JSCell* cell) {
cellsToMarkIfDoesCalls->append(cell);
});
}
return doesCalls;
}

Expand Down Expand Up @@ -1213,138 +1201,20 @@ void AccessCase::dump(PrintStream& out) const

bool AccessCase::visitWeak(VM& vm) const
{
switch (type()) {
case Getter:
case Setter: {
auto& accessor = this->as<GetterSetterAccessCase>();
if (accessor.callLinkInfo())
accessor.callLinkInfo()->visitWeak(vm);
break;
}
case ProxyObjectHas:
case ProxyObjectLoad:
case ProxyObjectStore:
case IndexedProxyObjectLoad: {
auto& accessor = this->as<ProxyObjectAccessCase>();
if (accessor.callLinkInfo())
accessor.callLinkInfo()->visitWeak(vm);
break;
}
case CustomValueGetter:
case CustomValueSetter:
case IntrinsicGetter:
case ModuleNamespaceLoad:
case InstanceOfHit:
case InstanceOfMiss:
case CustomAccessorGetter:
case CustomAccessorSetter:
case Load:
case LoadMegamorphic:
case StoreMegamorphic:
case InMegamorphic:
case Transition:
case Delete:
case DeleteNonConfigurable:
case DeleteMiss:
case Replace:
case Miss:
case GetGetter:
case InHit:
case InMiss:
case CheckPrivateBrand:
case SetPrivateBrand:
case ArrayLength:
case StringLength:
case DirectArgumentsLength:
case ScopedArgumentsLength:
case InstanceOfGeneric:
case IndexedMegamorphicLoad:
case IndexedMegamorphicStore:
case IndexedMegamorphicIn:
case IndexedInt32Load:
case IndexedDoubleLoad:
case IndexedContiguousLoad:
case IndexedArrayStorageLoad:
case IndexedScopedArgumentsLoad:
case IndexedDirectArgumentsLoad:
case IndexedTypedArrayInt8Load:
case IndexedTypedArrayUint8Load:
case IndexedTypedArrayUint8ClampedLoad:
case IndexedTypedArrayInt16Load:
case IndexedTypedArrayUint16Load:
case IndexedTypedArrayInt32Load:
case IndexedTypedArrayUint32Load:
case IndexedTypedArrayFloat32Load:
case IndexedTypedArrayFloat64Load:
case IndexedResizableTypedArrayInt8Load:
case IndexedResizableTypedArrayUint8Load:
case IndexedResizableTypedArrayUint8ClampedLoad:
case IndexedResizableTypedArrayInt16Load:
case IndexedResizableTypedArrayUint16Load:
case IndexedResizableTypedArrayInt32Load:
case IndexedResizableTypedArrayUint32Load:
case IndexedResizableTypedArrayFloat32Load:
case IndexedResizableTypedArrayFloat64Load:
case IndexedStringLoad:
case IndexedNoIndexingMiss:
case IndexedInt32Store:
case IndexedDoubleStore:
case IndexedContiguousStore:
case IndexedArrayStorageStore:
case IndexedTypedArrayInt8Store:
case IndexedTypedArrayUint8Store:
case IndexedTypedArrayUint8ClampedStore:
case IndexedTypedArrayInt16Store:
case IndexedTypedArrayUint16Store:
case IndexedTypedArrayInt32Store:
case IndexedTypedArrayUint32Store:
case IndexedTypedArrayFloat32Store:
case IndexedTypedArrayFloat64Store:
case IndexedResizableTypedArrayInt8Store:
case IndexedResizableTypedArrayUint8Store:
case IndexedResizableTypedArrayUint8ClampedStore:
case IndexedResizableTypedArrayInt16Store:
case IndexedResizableTypedArrayUint16Store:
case IndexedResizableTypedArrayInt32Store:
case IndexedResizableTypedArrayUint32Store:
case IndexedResizableTypedArrayFloat32Store:
case IndexedResizableTypedArrayFloat64Store:
case IndexedInt32InHit:
case IndexedDoubleInHit:
case IndexedContiguousInHit:
case IndexedArrayStorageInHit:
case IndexedScopedArgumentsInHit:
case IndexedDirectArgumentsInHit:
case IndexedTypedArrayInt8InHit:
case IndexedTypedArrayUint8InHit:
case IndexedTypedArrayUint8ClampedInHit:
case IndexedTypedArrayInt16InHit:
case IndexedTypedArrayUint16InHit:
case IndexedTypedArrayInt32InHit:
case IndexedTypedArrayUint32InHit:
case IndexedTypedArrayFloat32InHit:
case IndexedTypedArrayFloat64InHit:
case IndexedResizableTypedArrayInt8InHit:
case IndexedResizableTypedArrayUint8InHit:
case IndexedResizableTypedArrayUint8ClampedInHit:
case IndexedResizableTypedArrayInt16InHit:
case IndexedResizableTypedArrayUint16InHit:
case IndexedResizableTypedArrayInt32InHit:
case IndexedResizableTypedArrayUint32InHit:
case IndexedResizableTypedArrayFloat32InHit:
case IndexedResizableTypedArrayFloat64InHit:
case IndexedStringInHit:
case IndexedNoIndexingInMiss:
break;
}

bool isValid = true;
forEachDependentCell(vm, [&](JSCell* cell) {
isValid &= vm.heap.isMarked(cell);
});
return isValid;
}

void AccessCase::collectDependentCells(VM& vm, Vector<JSCell*>& cells) const
{
forEachDependentCell(vm, [&](JSCell* cell) {
cells.append(cell);
});
}

template<typename Visitor>
void AccessCase::propagateTransitions(Visitor& visitor) const
{
Expand Down
6 changes: 3 additions & 3 deletions Source/JavaScriptCore/bytecode/AccessCase.h
Original file line number Diff line number Diff line change
Expand Up @@ -224,9 +224,7 @@ class AccessCase : public ThreadSafeRefCounted<AccessCase> {
WatchpointSet* additionalSet() const;
bool viaGlobalProxy() const { return m_viaGlobalProxy; }

// If you supply the optional vector, this will append the set of cells that this will need to keep alive
// past the call.
bool doesCalls(VM&, Vector<JSCell*>* cellsToMark = nullptr) const;
bool doesCalls(VM&) const;

bool isCustom() const
{
Expand Down Expand Up @@ -294,6 +292,8 @@ class AccessCase : public ThreadSafeRefCounted<AccessCase> {

static bool canBeShared(const AccessCase&, const AccessCase&);

void collectDependentCells(VM&, Vector<JSCell*>&) const;

template<typename Func>
void runWithDowncast(const Func&);

Expand Down
8 changes: 3 additions & 5 deletions Source/JavaScriptCore/bytecode/GetByStatus.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ GetByStatus GetByStatus::computeForStubInfoWithoutExitSiteFeedback(const Concurr
auto& accessCase = access.as<ProxyObjectAccessCase>();
auto status = GetByStatus(accessCase);
auto callLinkStatus = makeUnique<CallLinkStatus>();
if (CallLinkInfo* callLinkInfo = accessCase.callLinkInfo())
if (CallLinkInfo* callLinkInfo = stubInfo->callLinkInfoAt(locker, 0))
*callLinkStatus = CallLinkStatus::computeFor(locker, profiledBlock, *callLinkInfo, callExitSiteData);
status.appendVariant(GetByVariant(accessCase.identifier(), { }, invalidOffset, { }, WTFMove(callLinkStatus)));
return status;
Expand Down Expand Up @@ -390,10 +390,8 @@ GetByStatus GetByStatus::computeForStubInfoWithoutExitSiteFeedback(const Concurr
}
case AccessCase::Getter: {
callLinkStatus = makeUnique<CallLinkStatus>();
if (CallLinkInfo* callLinkInfo = access.as<GetterSetterAccessCase>().callLinkInfo()) {
*callLinkStatus = CallLinkStatus::computeFor(
locker, profiledBlock, *callLinkInfo, callExitSiteData);
}
if (CallLinkInfo* callLinkInfo = stubInfo->callLinkInfoAt(locker, listIndex))
*callLinkStatus = CallLinkStatus::computeFor(locker, profiledBlock, *callLinkInfo, callExitSiteData);
break;
}
default: {
Expand Down
2 changes: 0 additions & 2 deletions Source/JavaScriptCore/bytecode/GetterSetterAccessCase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,6 @@ void GetterSetterAccessCase::dumpImpl(PrintStream& out, CommaPrinter& comma, Ind
{
Base::dumpImpl(out, comma, indent);
out.print(comma, "customSlotBase = ", RawPointer(customSlotBase()));
if (callLinkInfo())
out.print(comma, "callLinkInfo = ", RawPointer(callLinkInfo()));
out.print(comma, "customAccessor = ", RawPointer(m_customAccessor.taggedPtr()));
}

Expand Down
10 changes: 0 additions & 10 deletions Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,6 @@ class GetterSetterAccessCase final : public ProxyableAccessCase {
friend class AccessCase;
friend class InlineCacheCompiler;

// This can return null if it hasn't been generated yet. That's
// actually somewhat likely because of how we do buffering of new cases.
// CallLinkInfo's ownership is held both by generated code via GCAwareJITStubRoutine and PolymorphicAccess.
// The ownership relation is PolymorphicAccess -> GCAwareJITStubRoutine -> CallLinkInfo.
// PolymorphicAccess can be destroyed while GCAwareJITStubRoutine is alive if we are destroying PolymorphicAccess
// while we are executing GCAwareJITStubRoutine. It is not possible that GetterSetterAccessCase is alive while
// GCAwareJITStubRoutine is destroyed.
OptimizingCallLinkInfo* callLinkInfo() const { return m_callLinkInfo; }
JSObject* customSlotBase() const { return m_customSlotBase.get(); }
std::optional<DOMAttributeAnnotation> domAttribute() const { return m_domAttribute; }

Expand All @@ -69,9 +61,7 @@ class GetterSetterAccessCase final : public ProxyableAccessCase {
void dumpImpl(PrintStream&, CommaPrinter&, Indenter&) const;
Ref<AccessCase> cloneImpl() const;


WriteBarrier<JSObject> m_customSlotBase;
OptimizingCallLinkInfo* m_callLinkInfo { nullptr };
CodePtr<CustomAccessorPtrTag> m_customAccessor;
std::optional<DOMAttributeAnnotation> m_domAttribute;
};
Expand Down
Loading

0 comments on commit b251507

Please sign in to comment.