[webkit-changes] [280066] trunk/Source/JavaScriptCore

Mon, 19 Jul 2021 18:30:51 -0700

Title: [280066] trunk/Source/_javascript_Core
Revision
280066
Author
[email protected]
Date
2021-07-19 18:29:57 -0700 (Mon, 19 Jul 2021)

Log Message

[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:

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (280065 => 280066)


--- trunk/Source/_javascript_Core/ChangeLog	2021-07-20 01:27:39 UTC (rev 280065)
+++ trunk/Source/_javascript_Core/ChangeLog	2021-07-20 01:29:57 UTC (rev 280066)
@@ -1,3 +1,43 @@
+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-19  Mark Lam  <[email protected]>
 
         DFG's parseIntResult() should check for negative zero.

Modified: trunk/Source/_javascript_Core/bytecode/AccessCase.cpp (280065 => 280066)


--- trunk/Source/_javascript_Core/bytecode/AccessCase.cpp	2021-07-20 01:27:39 UTC (rev 280065)
+++ trunk/Source/_javascript_Core/bytecode/AccessCase.cpp	2021-07-20 01:29:57 UTC (rev 280066)
@@ -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: trunk/Source/_javascript_Core/bytecode/GetByStatus.cpp (280065 => 280066)


--- trunk/Source/_javascript_Core/bytecode/GetByStatus.cpp	2021-07-20 01:27:39 UTC (rev 280065)
+++ trunk/Source/_javascript_Core/bytecode/GetByStatus.cpp	2021-07-20 01:29:57 UTC (rev 280066)
@@ -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: trunk/Source/_javascript_Core/bytecode/InByStatus.cpp (280065 => 280066)


--- trunk/Source/_javascript_Core/bytecode/InByStatus.cpp	2021-07-20 01:27:39 UTC (rev 280065)
+++ trunk/Source/_javascript_Core/bytecode/InByStatus.cpp	2021-07-20 01:29:57 UTC (rev 280066)
@@ -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: trunk/Source/_javascript_Core/bytecode/PutByIdStatus.cpp (280065 => 280066)


--- trunk/Source/_javascript_Core/bytecode/PutByIdStatus.cpp	2021-07-20 01:27:39 UTC (rev 280065)
+++ trunk/Source/_javascript_Core/bytecode/PutByIdStatus.cpp	2021-07-20 01:29:57 UTC (rev 280066)
@@ -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: trunk/Source/_javascript_Core/bytecode/StructureStubInfo.cpp (280065 => 280066)


--- trunk/Source/_javascript_Core/bytecode/StructureStubInfo.cpp	2021-07-20 01:27:39 UTC (rev 280065)
+++ trunk/Source/_javascript_Core/bytecode/StructureStubInfo.cpp	2021-07-20 01:29:57 UTC (rev 280066)
@@ -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: trunk/Source/_javascript_Core/bytecode/StructureStubInfo.h (280065 => 280066)


--- trunk/Source/_javascript_Core/bytecode/StructureStubInfo.h	2021-07-20 01:27:39 UTC (rev 280065)
+++ trunk/Source/_javascript_Core/bytecode/StructureStubInfo.h	2021-07-20 01:29:57 UTC (rev 280066)
@@ -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.

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to