Skip to content

Commit b7b433a

Browse files
kmiller68kmiller68
committed
[Wasm] GC structs shouldn't need to visit fields if there's no ref fields
https://bugs.webkit.org/show_bug.cgi?id=289112 rdar://146141202 Reviewed by Yusuke Suzuki. There's a couple of changes in this PR that didn't make it into 291579@main: 1) GC structs shouldn't walk the fields looking for ref type fields unless there's at least one field with a ref type. In Dart-flutter-wasm a decent number of the structs have no outgoing pointers, which also seems likely to be the case in general. In the future we could also cache the offsets of outgoing pointers in the `Wasm::StructType` but since this didn't show any progression we should make sure it's worthwhile to use the extra memory. 2) Make `RTT` a `TrailingArray` subclass. 3) Rename `RTT::isSubRTT` to `RTT::isStrictSubRTT` and `RTT::isSubRTTWithEquality` to `RTT::isSubRTT`, which better matches the general programming language parlance. As mentioned above this didn't seem to improve the benchmark but doesn't cost us any memory so it seems worthwhile to just land. * Source/JavaScriptCore/wasm/WasmFormat.cpp: (JSC::Wasm::validateWasmValue): * Source/JavaScriptCore/wasm/WasmFormat.h: (JSC::Wasm::isSubtypeIndex): * Source/JavaScriptCore/wasm/WasmOMGIRGenerator.cpp: (JSC::Wasm::OMGIRGenerator::emitRefTestOrCast): * Source/JavaScriptCore/wasm/WasmOMGIRGenerator32_64.cpp: (JSC::Wasm::OMGIRGenerator::emitRefTestOrCast): * Source/JavaScriptCore/wasm/WasmOperations.cpp: (JSC::Wasm::JSC_DEFINE_NOEXCEPT_JIT_OPERATION): * Source/JavaScriptCore/wasm/WasmOperations.h: * Source/JavaScriptCore/wasm/WasmTypeDefinition.cpp: (JSC::Wasm::StructType::StructType): (JSC::Wasm::RTT::isStrictSubRTT const): (JSC::Wasm::TypeInformation::castReference): (JSC::Wasm::RTT::isSubRTT const): Deleted. * Source/JavaScriptCore/wasm/WasmTypeDefinition.h: (JSC::Wasm::StructType::field const): (JSC::Wasm::StructType::hasRefFieldTypes const): (JSC::Wasm::StructType::setHasRecursiveReference): (JSC::Wasm:: const): * Source/JavaScriptCore/wasm/js/JSWebAssemblyStruct.cpp: (JSC::JSWebAssemblyStruct::visitChildrenImpl): Canonical link: https://commits.webkit.org/291603@main
1 parent 9ce32ac commit b7b433a

9 files changed

Lines changed: 43 additions & 36 deletions

‎Source/JavaScriptCore/wasm/WasmFormat.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ void validateWasmValue(uint64_t wasmValue, Type expectedType)
9494
}
9595
auto objectPtr = jsCast<WebAssemblyGCObjectBase*>(value);
9696
RefPtr<const RTT> objectRTT = objectPtr->rtt();
97-
ASSERT(objectRTT->isSubRTTWithEquality(*expectedRTT.get()));
97+
ASSERT(objectRTT->isSubRTT(*expectedRTT.get()));
9898
}
9999
}
100100
}

‎Source/JavaScriptCore/wasm/WasmFormat.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,7 @@ inline bool isSubtypeIndex(TypeIndex sub, TypeIndex parent)
291291
auto parentRTT = TypeInformation::tryGetCanonicalRTT(parent);
292292
ASSERT(subRTT.has_value() && parentRTT.has_value());
293293

294-
return subRTT.value()->isSubRTT(*parentRTT.value());
294+
return subRTT.value()->isStrictSubRTT(*parentRTT.value());
295295
}
296296

297297
bool isSubtype(Type, Type);

‎Source/JavaScriptCore/wasm/WasmOMGIRGenerator.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3716,7 +3716,7 @@ void OMGIRGenerator::emitRefTestOrCast(CastKind castKind, ExpressionType referen
37163716
slowPath->addPredecessor(m_currentBlock);
37173717

37183718
m_currentBlock = slowPath;
3719-
Value* isSubRTT = callWasmOperation(m_currentBlock, B3::Int32, operationWasmIsSubRTT, rtt, targetRTT);
3719+
Value* isSubRTT = callWasmOperation(m_currentBlock, B3::Int32, operationWasmIsStrictSubRTT, rtt, targetRTT);
37203720
emitCheckOrBranchForCast(castKind, m_currentBlock->appendNew<Value>(m_proc, Equal, origin(), isSubRTT, constant(Int32, 0)), castFailure, falseBlock);
37213721
}
37223722

‎Source/JavaScriptCore/wasm/WasmOMGIRGenerator32_64.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4003,7 +4003,7 @@ void OMGIRGenerator::emitRefTestOrCast(CastKind castKind, ExpressionType referen
40034003
m_currentBlock = slowPath;
40044004
// FIXME: It may be worthwhile to JIT inline this in the future.
40054005
Value* isSubRTT = append<CCallValue>(m_proc, B3::Int32, origin(),
4006-
append<ConstPtrValue>(m_proc, origin(), tagCFunction<OperationPtrTag>(operationWasmIsSubRTT)),
4006+
append<ConstPtrValue>(m_proc, origin(), tagCFunction<OperationPtrTag>(operationWasmIsStrictSubRTT)),
40074007
truncate(rtt), targetRTT);
40084008
emitCheckOrBranchForCast(castKind, append<Value>(m_proc, Equal, origin(), isSubRTT, constant(Int32, 0)), castFailure, falseBlock);
40094009
}

‎Source/JavaScriptCore/wasm/WasmOperations.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1874,10 +1874,10 @@ JSC_DEFINE_NOEXCEPT_JIT_OPERATION(operationWasmArrayInitData, UCPUStrictInt32, (
18741874
return toUCPUStrictInt32(arrayInitData(instance, dst, dstOffset, srcDataIndex, srcOffset, size));
18751875
}
18761876

1877-
JSC_DEFINE_NOEXCEPT_JIT_OPERATION(operationWasmIsSubRTT, bool, (RTT* maybeSubRTT, RTT* targetRTT))
1877+
JSC_DEFINE_NOEXCEPT_JIT_OPERATION(operationWasmIsStrictSubRTT, bool, (RTT* maybeSubRTT, RTT* targetRTT))
18781878
{
18791879
ASSERT(maybeSubRTT && targetRTT);
1880-
return maybeSubRTT->isSubRTT(*targetRTT);
1880+
return maybeSubRTT->isStrictSubRTT(*targetRTT);
18811881
}
18821882

18831883
JSC_DEFINE_NOEXCEPT_JIT_OPERATION(operationWasmAnyConvertExtern, EncodedJSValue, (EncodedJSValue reference))

‎Source/JavaScriptCore/wasm/WasmOperations.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ JSC_DECLARE_NOEXCEPT_JIT_OPERATION(operationWasmArrayFillVector, UCPUStrictInt32
135135
JSC_DECLARE_NOEXCEPT_JIT_OPERATION(operationWasmArrayCopy, UCPUStrictInt32, (JSWebAssemblyInstance*, EncodedJSValue, uint32_t, EncodedJSValue, uint32_t, uint32_t));
136136
JSC_DECLARE_NOEXCEPT_JIT_OPERATION(operationWasmArrayInitElem, UCPUStrictInt32, (JSWebAssemblyInstance*, EncodedJSValue, uint32_t, uint32_t, uint32_t, uint32_t));
137137
JSC_DECLARE_NOEXCEPT_JIT_OPERATION(operationWasmArrayInitData, UCPUStrictInt32, (JSWebAssemblyInstance*, EncodedJSValue, uint32_t, uint32_t, uint32_t, uint32_t));
138-
JSC_DECLARE_NOEXCEPT_JIT_OPERATION(operationWasmIsSubRTT, bool, (Wasm::RTT*, Wasm::RTT*));
138+
JSC_DECLARE_NOEXCEPT_JIT_OPERATION(operationWasmIsStrictSubRTT, bool, (Wasm::RTT*, Wasm::RTT*));
139139
JSC_DECLARE_NOEXCEPT_JIT_OPERATION(operationWasmAnyConvertExtern, EncodedJSValue, (EncodedJSValue));
140140

141141
JSC_DECLARE_NOEXCEPT_JIT_OPERATION(operationWasmRefTest, int32_t, (JSWebAssemblyInstance*, EncodedJSValue, uint32_t, int32_t, bool));

‎Source/JavaScriptCore/wasm/WasmTypeDefinition.cpp‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -124,13 +124,13 @@ void StructType::dump(PrintStream& out) const
124124
StructType::StructType(void* payload, StructFieldCount fieldCount, const FieldType* fieldTypes)
125125
: m_payload(static_cast<FieldType*>(payload))
126126
, m_fieldCount(fieldCount)
127-
, m_hasRecursiveReference(false)
128127
{
129-
bool hasRecursiveReference = false;
130128
unsigned currentFieldOffset = 0;
131129
for (unsigned fieldIndex = 0; fieldIndex < m_fieldCount; ++fieldIndex) {
132130
const auto& fieldType = fieldTypes[fieldIndex];
133-
hasRecursiveReference |= isRefWithRecursiveReference(fieldType.type);
131+
m_hasRefFieldTypes |= isRefType(fieldType.type);
132+
m_hasRecursiveReference |= isRefWithRecursiveReference(fieldType.type);
133+
134134
getField(fieldIndex) = fieldType;
135135

136136
const auto& fieldStorageType = field(fieldIndex).type;
@@ -140,7 +140,6 @@ StructType::StructType(void* payload, StructFieldCount fieldCount, const FieldTy
140140
}
141141

142142
m_instancePayloadSize = WTF::roundUpToMultipleOf<sizeof(uint64_t)>(currentFieldOffset);
143-
setHasRecursiveReference(hasRecursiveReference);
144143
}
145144

146145
String ArrayType::toString() const
@@ -582,7 +581,7 @@ RefPtr<RTT> RTT::tryCreateRTT(RTTKind kind, DisplayCount displaySize)
582581
return adoptRef(new (NotNull, memory) RTT(kind, displaySize));
583582
}
584583

585-
bool RTT::isSubRTT(const RTT& parent) const
584+
bool RTT::isStrictSubRTT(const RTT& parent) const
586585
{
587586
if (displaySize() > 0) {
588587
if (parent.displaySize() > 0) {
@@ -1143,7 +1142,7 @@ bool TypeInformation::castReference(JSValue refValue, bool allowNull, TypeIndex
11431142
auto funcRTT = funcRef->rtt();
11441143
if (funcRTT == signatureRTT.get())
11451144
return true;
1146-
return funcRTT->isSubRTT(*signatureRTT);
1145+
return funcRTT->isStrictSubRTT(*signatureRTT);
11471146
}
11481147
if (signature.is<ArrayType>()) {
11491148
JSWebAssemblyArray* arrayRef = jsDynamicCast<JSWebAssemblyArray*>(refValue);
@@ -1152,7 +1151,7 @@ bool TypeInformation::castReference(JSValue refValue, bool allowNull, TypeIndex
11521151
auto arrayRTT = arrayRef->rtt();
11531152
if (arrayRTT.get() == signatureRTT.get())
11541153
return true;
1155-
return arrayRTT->isSubRTT(*signatureRTT);
1154+
return arrayRTT->isStrictSubRTT(*signatureRTT);
11561155
}
11571156
ASSERT(signature.is<StructType>());
11581157
JSWebAssemblyStruct* structRef = jsDynamicCast<JSWebAssemblyStruct*>(refValue);
@@ -1161,7 +1160,7 @@ bool TypeInformation::castReference(JSValue refValue, bool allowNull, TypeIndex
11611160
auto structRTT = structRef->rtt();
11621161
if (structRTT.get() == signatureRTT.get())
11631162
return true;
1164-
return structRTT->isSubRTT(*signatureRTT);
1163+
return structRTT->isStrictSubRTT(*signatureRTT);
11651164
}
11661165

11671166
return false;

‎Source/JavaScriptCore/wasm/WasmTypeDefinition.h‎

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -569,9 +569,11 @@ class StructType {
569569
StructType(void*, StructFieldCount, const FieldType*);
570570

571571
StructFieldCount fieldCount() const { return m_fieldCount; }
572+
FieldType field(StructFieldCount i) const { return const_cast<StructType*>(this)->getField(i); }
573+
574+
bool hasRefFieldTypes() const { return m_hasRefFieldTypes; }
572575
bool hasRecursiveReference() const { return m_hasRecursiveReference; }
573576
void setHasRecursiveReference(bool value) { m_hasRecursiveReference = value; }
574-
FieldType field(StructFieldCount i) const { return const_cast<StructType*>(this)->getField(i); }
575577

576578
WTF::String toString() const;
577579
void dump(WTF::PrintStream& out) const;
@@ -590,7 +592,9 @@ class StructType {
590592
// Payload is structured this way = | field types | precalculated field offsets |.
591593
FieldType* m_payload;
592594
StructFieldCount m_fieldCount;
593-
bool m_hasRecursiveReference;
595+
// FIXME: We should consider caching the offsets of exactly which fields are ref types in m_payload to speed up visitChildren.
596+
bool m_hasRefFieldTypes { false };
597+
bool m_hasRecursiveReference { false };
594598
size_t m_instancePayloadSize;
595599
};
596600

@@ -741,40 +745,36 @@ enum class RTTKind : uint8_t {
741745
Struct
742746
};
743747

744-
class RTT_ALIGNMENT RTT : public ThreadSafeRefCounted<RTT> {
748+
class RTT_ALIGNMENT RTT final : public ThreadSafeRefCounted<RTT>, private TrailingArray<RTT, const RTT*> {
745749
WTF_MAKE_FAST_COMPACT_ALLOCATED;
746-
750+
WTF_MAKE_NONMOVABLE(RTT);
751+
using TrailingArrayType = TrailingArray<RTT, const RTT*>;
752+
friend TrailingArrayType;
747753
public:
748754
RTT() = delete;
749-
RTT(const RTT&) = delete;
750-
751-
explicit RTT(RTTKind kind, DisplayCount displaySize)
752-
: m_kind(kind)
753-
, m_displaySize(displaySize)
754-
{
755-
}
756755

757756
static RefPtr<RTT> tryCreateRTT(RTTKind, DisplayCount);
758757

759758
RTTKind kind() const { return m_kind; }
760-
DisplayCount displaySize() const { return m_displaySize; }
761-
const RTT* displayEntry(DisplayCount i) const { ASSERT(i < displaySize()); return const_cast<RTT*>(this)->payload()[i]; }
762-
void setDisplayEntry(DisplayCount i, RefPtr<const RTT> entry) { ASSERT(i < displaySize()); payload()[i] = entry.get(); }
759+
DisplayCount displaySize() const { return size(); }
760+
const RTT* displayEntry(DisplayCount i) const { return at(i); }
761+
void setDisplayEntry(DisplayCount i, RefPtr<const RTT> entry) { at(i) = entry.get(); }
763762

764-
bool isSubRTTWithEquality(const RTT& other) const { return this == &other ? true : isSubRTT(other); }
765-
bool isSubRTT(const RTT& other) const;
763+
bool isSubRTT(const RTT& other) const { return this == &other ? true : isStrictSubRTT(other); }
764+
bool isStrictSubRTT(const RTT& other) const;
766765
static size_t allocatedRTTSize(Checked<DisplayCount> count) { return sizeof(RTT) + count * sizeof(TypeIndex); }
767766

768767
static constexpr ptrdiff_t offsetOfKind() { return OBJECT_OFFSETOF(RTT, m_kind); }
769-
static constexpr ptrdiff_t offsetOfDisplaySize() { return OBJECT_OFFSETOF(RTT, m_displaySize); }
770-
static constexpr ptrdiff_t offsetOfPayload() { return sizeof(RTT); }
768+
static constexpr ptrdiff_t offsetOfDisplaySize() { return offsetOfSize(); }
769+
static constexpr ptrdiff_t offsetOfPayload() { return offsetOfData(); }
771770

772771
private:
773-
// Payload starts past end of this object.
774-
const RTT** payload() { return static_cast<const RTT**>(static_cast<void*>(this + 1)); }
772+
explicit RTT(RTTKind kind, DisplayCount displaySize)
773+
: TrailingArrayType(displaySize)
774+
, m_kind(kind)
775+
{ }
775776

776777
RTTKind m_kind;
777-
DisplayCount m_displaySize;
778778
};
779779

780780
enum class TypeDefinitionKind : uint8_t {

‎Source/JavaScriptCore/wasm/js/JSWebAssemblyStruct.cpp‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,14 @@ void JSWebAssemblyStruct::visitChildrenImpl(JSCell* cell, Visitor& visitor)
175175
Base::visitChildren(cell, visitor);
176176

177177
auto* wasmStruct = jsCast<JSWebAssemblyStruct*>(cell);
178+
if (!wasmStruct->structType().hasRefFieldTypes()) {
179+
#if ASSERT_ENABLED
180+
for (unsigned i = 0; i < wasmStruct->structType().fieldCount(); ++i)
181+
ASSERT(!isRefType(wasmStruct->fieldType(i).type));
182+
#endif
183+
return;
184+
}
185+
178186
for (unsigned i = 0; i < wasmStruct->structType().fieldCount(); ++i) {
179187
auto fieldType = wasmStruct->fieldType(i).type;
180188
if (isRefType(fieldType)) {

0 commit comments

Comments
 (0)