Diff
Modified: branches/safari-612.1.24.11-branch/Source/_javascript_Core/ChangeLog (280186 => 280187)
--- branches/safari-612.1.24.11-branch/Source/_javascript_Core/ChangeLog 2021-07-22 18:10:43 UTC (rev 280186)
+++ branches/safari-612.1.24.11-branch/Source/_javascript_Core/ChangeLog 2021-07-22 18:10:47 UTC (rev 280187)
@@ -1,5 +1,89 @@
2021-07-20 Ruben Turcios <[email protected]>
+ Cherry-pick r280066. rdar://problem/80851562
+
+ [JSC] StructureStubInfo's m_identifier should follow to the same protocol of inlineAccessBaseStructure
+ https://bugs.webkit.org/show_bug.cgi?id=228092
+
+ Reviewed by Saam Barati.
+
+ In r279813, we fixed a race condition related to inlineAccessBaseStructure: while we clear inlineAccessBaseStructure,
+ we still run code relying on this field's value until stub version of the code is generated. As a result,
+ we run the code which relies on the cells that are already collected. And we have the same problem with
+ m_identifier field too. This patch makes m_identifier follow to the same protocol of inlineAccessBaseStructure
+ so that we fix this race issue too: both fields will be alive until we switch to the code that are not relying on these
+ fields.
+
+ We also make inlineAccessBaseStructure to m_inlineAccessBaseStructure to easily find that this is member field.
+ And we also use setWithoutWriteBarrier for m_inlineAccessBaseStructure since we emit codeBlock->vm().heap.writeBarrier(codeBlock)
+ immediately after that.
+
+ * bytecode/AccessCase.cpp:
+ (JSC::AccessCase::fromStructureStubInfo):
+ * bytecode/GetByStatus.cpp:
+ (JSC::GetByStatus::computeForStubInfoWithoutExitSiteFeedback):
+ * bytecode/InByStatus.cpp:
+ (JSC::InByStatus::computeForStubInfoWithoutExitSiteFeedback):
+ * bytecode/PutByIdStatus.cpp:
+ (JSC::PutByIdStatus::computeForStubInfo):
+ * bytecode/StructureStubInfo.cpp:
+ (JSC::StructureStubInfo::initGetByIdSelf):
+ (JSC::StructureStubInfo::initArrayLength):
+ (JSC::StructureStubInfo::initStringLength):
+ (JSC::StructureStubInfo::initPutByIdReplace):
+ (JSC::StructureStubInfo::initInByIdSelf):
+ (JSC::StructureStubInfo::addAccessCase):
+ (JSC::StructureStubInfo::reset):
+ (JSC::StructureStubInfo::visitAggregateImpl):
+ (JSC::StructureStubInfo::visitWeakReferences):
+ (JSC::StructureStubInfo::propagateTransitions):
+ (JSC::StructureStubInfo::setCacheType): Deleted.
+ * bytecode/StructureStubInfo.h:
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@280066 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2021-07-19 Yusuke Suzuki <[email protected]>
+
+ [JSC] StructureStubInfo's m_identifier should follow to the same protocol of inlineAccessBaseStructure
+ https://bugs.webkit.org/show_bug.cgi?id=228092
+
+ Reviewed by Saam Barati.
+
+ In r279813, we fixed a race condition related to inlineAccessBaseStructure: while we clear inlineAccessBaseStructure,
+ we still run code relying on this field's value until stub version of the code is generated. As a result,
+ we run the code which relies on the cells that are already collected. And we have the same problem with
+ m_identifier field too. This patch makes m_identifier follow to the same protocol of inlineAccessBaseStructure
+ so that we fix this race issue too: both fields will be alive until we switch to the code that are not relying on these
+ fields.
+
+ We also make inlineAccessBaseStructure to m_inlineAccessBaseStructure to easily find that this is member field.
+ And we also use setWithoutWriteBarrier for m_inlineAccessBaseStructure since we emit codeBlock->vm().heap.writeBarrier(codeBlock)
+ immediately after that.
+
+ * bytecode/AccessCase.cpp:
+ (JSC::AccessCase::fromStructureStubInfo):
+ * bytecode/GetByStatus.cpp:
+ (JSC::GetByStatus::computeForStubInfoWithoutExitSiteFeedback):
+ * bytecode/InByStatus.cpp:
+ (JSC::InByStatus::computeForStubInfoWithoutExitSiteFeedback):
+ * bytecode/PutByIdStatus.cpp:
+ (JSC::PutByIdStatus::computeForStubInfo):
+ * bytecode/StructureStubInfo.cpp:
+ (JSC::StructureStubInfo::initGetByIdSelf):
+ (JSC::StructureStubInfo::initArrayLength):
+ (JSC::StructureStubInfo::initStringLength):
+ (JSC::StructureStubInfo::initPutByIdReplace):
+ (JSC::StructureStubInfo::initInByIdSelf):
+ (JSC::StructureStubInfo::addAccessCase):
+ (JSC::StructureStubInfo::reset):
+ (JSC::StructureStubInfo::visitAggregateImpl):
+ (JSC::StructureStubInfo::visitWeakReferences):
+ (JSC::StructureStubInfo::propagateTransitions):
+ (JSC::StructureStubInfo::setCacheType): Deleted.
+ * bytecode/StructureStubInfo.h:
+
+2021-07-20 Ruben Turcios <[email protected]>
+
Cherry-pick r280050. rdar://problem/80851606
[JSC] InByStatus / InByVariant should visit CacheableIdentifier
Modified: branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/AccessCase.cpp (280186 => 280187)
--- branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/AccessCase.cpp 2021-07-22 18:10:43 UTC (rev 280186)
+++ branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/AccessCase.cpp 2021-07-22 18:10:47 UTC (rev 280187)
@@ -155,15 +155,15 @@
switch (stubInfo.cacheType()) {
case CacheType::GetByIdSelf:
RELEASE_ASSERT(stubInfo.hasConstantIdentifier);
- return ProxyableAccessCase::create(vm, owner, Load, identifier, stubInfo.u.byIdSelf.offset, stubInfo.inlineAccessBaseStructure.get());
+ return ProxyableAccessCase::create(vm, owner, Load, identifier, stubInfo.u.byIdSelf.offset, stubInfo.m_inlineAccessBaseStructure.get());
case CacheType::PutByIdReplace:
RELEASE_ASSERT(stubInfo.hasConstantIdentifier);
- return AccessCase::create(vm, owner, Replace, identifier, stubInfo.u.byIdSelf.offset, stubInfo.inlineAccessBaseStructure.get());
+ return AccessCase::create(vm, owner, Replace, identifier, stubInfo.u.byIdSelf.offset, stubInfo.m_inlineAccessBaseStructure.get());
case CacheType::InByIdSelf:
RELEASE_ASSERT(stubInfo.hasConstantIdentifier);
- return AccessCase::create(vm, owner, InHit, identifier, stubInfo.u.byIdSelf.offset, stubInfo.inlineAccessBaseStructure.get());
+ return AccessCase::create(vm, owner, InHit, identifier, stubInfo.u.byIdSelf.offset, stubInfo.m_inlineAccessBaseStructure.get());
case CacheType::ArrayLength:
RELEASE_ASSERT(stubInfo.hasConstantIdentifier);
Modified: branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/GetByStatus.cpp (280186 => 280187)
--- branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/GetByStatus.cpp 2021-07-22 18:10:43 UTC (rev 280186)
+++ branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/GetByStatus.cpp 2021-07-22 18:10:47 UTC (rev 280187)
@@ -213,7 +213,7 @@
return GetByStatus(NoInformation);
case CacheType::GetByIdSelf: {
- Structure* structure = stubInfo->inlineAccessBaseStructure.get();
+ Structure* structure = stubInfo->m_inlineAccessBaseStructure.get();
if (structure->takesSlowPathInDFGForImpureProperty())
return GetByStatus(JSC::slowVersion(summary), *stubInfo);
CacheableIdentifier identifier = stubInfo->identifier();
Modified: branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/InByStatus.cpp (280186 => 280187)
--- branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/InByStatus.cpp 2021-07-22 18:10:43 UTC (rev 280186)
+++ branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/InByStatus.cpp 2021-07-22 18:10:47 UTC (rev 280187)
@@ -139,7 +139,7 @@
return InByStatus(NoInformation);
case CacheType::InByIdSelf: {
- Structure* structure = stubInfo->inlineAccessBaseStructure.get();
+ Structure* structure = stubInfo->m_inlineAccessBaseStructure.get();
if (structure->takesSlowPathInDFGForImpureProperty())
return InByStatus(TakesSlowPath);
CacheableIdentifier identifier = stubInfo->identifier();
Modified: branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/PutByIdStatus.cpp (280186 => 280187)
--- branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/PutByIdStatus.cpp 2021-07-22 18:10:43 UTC (rev 280186)
+++ branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/PutByIdStatus.cpp 2021-07-22 18:10:47 UTC (rev 280187)
@@ -147,10 +147,10 @@
case CacheType::PutByIdReplace: {
PropertyOffset offset =
- stubInfo->inlineAccessBaseStructure->getConcurrently(uid);
+ stubInfo->m_inlineAccessBaseStructure->getConcurrently(uid);
if (isValidOffset(offset)) {
return PutByIdVariant::replace(
- stubInfo->inlineAccessBaseStructure.get(), offset);
+ stubInfo->m_inlineAccessBaseStructure.get(), offset);
}
return PutByIdStatus(JSC::slowVersion(summary));
}
Modified: branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/StructureStubInfo.cpp (280186 => 280187)
--- branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/StructureStubInfo.cpp 2021-07-22 18:10:43 UTC (rev 280186)
+++ branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/StructureStubInfo.cpp 2021-07-22 18:10:47 UTC (rev 280187)
@@ -61,42 +61,44 @@
void StructureStubInfo::initGetByIdSelf(const ConcurrentJSLockerBase& locker, CodeBlock* codeBlock, Structure* inlineAccessBaseStructure, PropertyOffset offset, CacheableIdentifier identifier)
{
+ ASSERT(m_cacheType == CacheType::Unset);
ASSERT(hasConstantIdentifier);
setCacheType(locker, CacheType::GetByIdSelf);
m_identifier = identifier;
+ m_inlineAccessBaseStructure.setWithoutWriteBarrier(inlineAccessBaseStructure);
codeBlock->vm().heap.writeBarrier(codeBlock);
-
- this->inlineAccessBaseStructure.set(codeBlock->vm(), codeBlock, inlineAccessBaseStructure);
u.byIdSelf.offset = offset;
}
void StructureStubInfo::initArrayLength(const ConcurrentJSLockerBase& locker)
{
+ ASSERT(m_cacheType == CacheType::Unset);
setCacheType(locker, CacheType::ArrayLength);
}
void StructureStubInfo::initStringLength(const ConcurrentJSLockerBase& locker)
{
+ ASSERT(m_cacheType == CacheType::Unset);
setCacheType(locker, CacheType::StringLength);
}
void StructureStubInfo::initPutByIdReplace(const ConcurrentJSLockerBase& locker, CodeBlock* codeBlock, Structure* inlineAccessBaseStructure, PropertyOffset offset, CacheableIdentifier identifier)
{
+ ASSERT(m_cacheType == CacheType::Unset);
setCacheType(locker, CacheType::PutByIdReplace);
m_identifier = identifier;
+ m_inlineAccessBaseStructure.setWithoutWriteBarrier(inlineAccessBaseStructure);
codeBlock->vm().heap.writeBarrier(codeBlock);
-
- this->inlineAccessBaseStructure.set(codeBlock->vm(), codeBlock, inlineAccessBaseStructure);
u.byIdSelf.offset = offset;
}
void StructureStubInfo::initInByIdSelf(const ConcurrentJSLockerBase& locker, CodeBlock* codeBlock, Structure* inlineAccessBaseStructure, PropertyOffset offset, CacheableIdentifier identifier)
{
+ ASSERT(m_cacheType == CacheType::Unset);
setCacheType(locker, CacheType::InByIdSelf);
m_identifier = identifier;
+ m_inlineAccessBaseStructure.setWithoutWriteBarrier(inlineAccessBaseStructure);
codeBlock->vm().heap.writeBarrier(codeBlock);
-
- this->inlineAccessBaseStructure.set(codeBlock->vm(), codeBlock, inlineAccessBaseStructure);
u.byIdSelf.offset = offset;
}
@@ -230,10 +232,11 @@
// access code. That's because when we first transition to becoming a Stub, we may
// be buffered, and we have not yet generated any code. Once the Stub finally generates
// code, we're no longer running the inline access code, so we can then clear out
- // inlineAccessBaseStructure. The reason we don't clear inlineAccessBaseStructure while
- // we're buffered is because we rely on it to reset during GC if inlineAccessBaseStructure
+ // m_inlineAccessBaseStructure. The reason we don't clear m_inlineAccessBaseStructure while
+ // we're buffered is because we rely on it to reset during GC if m_inlineAccessBaseStructure
// is collected.
- inlineAccessBaseStructure.clear();
+ m_identifier = nullptr;
+ m_inlineAccessBaseStructure.clear();
// If we generated some code then we don't want to attempt to repatch in the future until we
// gather enough cases.
@@ -247,7 +250,8 @@
void StructureStubInfo::reset(const ConcurrentJSLockerBase& locker, CodeBlock* codeBlock)
{
clearBufferedStructures();
- inlineAccessBaseStructure.clear();
+ m_identifier = nullptr;
+ m_inlineAccessBaseStructure.clear();
if (m_cacheType == CacheType::Unset)
return;
@@ -321,15 +325,14 @@
for (auto& bufferedStructure : m_bufferedStructures)
bufferedStructure.byValId().visitAggregate(visitor);
}
+ m_identifier.visitAggregate(visitor);
switch (m_cacheType) {
case CacheType::Unset:
case CacheType::ArrayLength:
case CacheType::StringLength:
- return;
case CacheType::PutByIdReplace:
case CacheType::InByIdSelf:
case CacheType::GetByIdSelf:
- m_identifier.visitAggregate(visitor);
return;
case CacheType::Stub:
u.stub->visitAggregate(visitor);
@@ -354,8 +357,8 @@
}
bool isValid = true;
- if (inlineAccessBaseStructure)
- isValid &= vm.heap.isMarked(inlineAccessBaseStructure.get());
+ if (m_inlineAccessBaseStructure)
+ isValid &= vm.heap.isMarked(m_inlineAccessBaseStructure.get());
if (m_cacheType == CacheType::Stub)
isValid &= u.stub->visitWeak(vm);
@@ -369,8 +372,8 @@
template<typename Visitor>
void StructureStubInfo::propagateTransitions(Visitor& visitor)
{
- if (inlineAccessBaseStructure)
- inlineAccessBaseStructure->markIfCheap(visitor);
+ if (m_inlineAccessBaseStructure)
+ m_inlineAccessBaseStructure->markIfCheap(visitor);
if (m_cacheType == CacheType::Stub)
u.stub->propagateTransitions(visitor);
@@ -419,20 +422,8 @@
return u.stub->containsPC(pc);
}
-void StructureStubInfo::setCacheType(const ConcurrentJSLockerBase&, CacheType newCacheType)
+ALWAYS_INLINE void StructureStubInfo::setCacheType(const ConcurrentJSLockerBase&, CacheType newCacheType)
{
- switch (m_cacheType) {
- case CacheType::Unset:
- case CacheType::ArrayLength:
- case CacheType::StringLength:
- case CacheType::Stub:
- break;
- case CacheType::PutByIdReplace:
- case CacheType::InByIdSelf:
- case CacheType::GetByIdSelf:
- m_identifier = nullptr;
- break;
- }
m_cacheType = newCacheType;
}
Modified: branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/StructureStubInfo.h (280186 => 280187)
--- branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/StructureStubInfo.h 2021-07-22 18:10:43 UTC (rev 280186)
+++ branches/safari-612.1.24.11-branch/Source/_javascript_Core/bytecode/StructureStubInfo.h 2021-07-22 18:10:47 UTC (rev 280187)
@@ -330,7 +330,7 @@
} byIdSelf;
PolymorphicAccess* stub;
} u;
- WriteBarrier<Structure> inlineAccessBaseStructure;
+ WriteBarrier<Structure> m_inlineAccessBaseStructure;
private:
CacheableIdentifier m_identifier;
// Represents those structures that already have buffered AccessCases in the PolymorphicAccess.
