fourthTier: DFG should be able to query Structure without modifying it
authoroliver@apple.com <oliver@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 Jul 2013 03:58:27 +0000 (03:58 +0000)
committeroliver@apple.com <oliver@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 Jul 2013 03:58:27 +0000 (03:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=114708

Reviewed by Oliver Hunt.

This is work towards allowing the DFG, and FTL, to run on a separate thread.
The idea is that the most evil thing that the DFG does that has thread-safety
issues is fiddling with Structures by calling Structure::get(). This can lead
to rematerialization of property tables, which is definitely not thread-safe
due to how StringImpl works. So, this patch completely side-steps the problem
by creating a new version of Structure::get, called
Structure::getWithoutMaterializing, which may choose to do an O(n) search if
necessary to avoid materialization. I believe this should be fine - the DFG
does't call into these code path often enough for this to matter, and most of
the time, the Structure that we call this on will already have a property
table because some inline cache would have already called ::get() on that
Structure.

Also cleaned up the materialization logic: we can stop the search as soon as
we find any Structure with a property table rather than searching all the way
for a pinned one.

* bytecode/GetByIdStatus.cpp:
(JSC::GetByIdStatus::computeFor):
* bytecode/PutByIdStatus.cpp:
(JSC::PutByIdStatus::computeFromLLInt):
(JSC::PutByIdStatus::computeFor):
* runtime/Structure.cpp:
(JSC::Structure::findStructuresAndMapForMaterialization):
(JSC::Structure::materializePropertyMap):
(JSC::Structure::getWithoutMaterializing):
(JSC):
* runtime/Structure.h:
(Structure):
* runtime/StructureInlines.h:
(JSC::Structure::getWithoutMaterializing):
(JSC):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@153120 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/GetByIdStatus.cpp
Source/JavaScriptCore/bytecode/PutByIdStatus.cpp
Source/JavaScriptCore/runtime/Structure.cpp
Source/JavaScriptCore/runtime/Structure.h
Source/JavaScriptCore/runtime/StructureInlines.h

index b46c687..a847bf9 100644 (file)
@@ -1,3 +1,47 @@
+2013-07-16  Oliver Hunt  <oliver@apple.com>
+
+        Merged dfgFourthTier r148570
+
+    2013-04-16  Filip Pizlo  <fpizlo@apple.com>
+
+        fourthTier: DFG should be able to query Structure without modifying it
+        https://bugs.webkit.org/show_bug.cgi?id=114708
+
+        Reviewed by Oliver Hunt.
+        
+        This is work towards allowing the DFG, and FTL, to run on a separate thread.
+        The idea is that the most evil thing that the DFG does that has thread-safety
+        issues is fiddling with Structures by calling Structure::get(). This can lead
+        to rematerialization of property tables, which is definitely not thread-safe
+        due to how StringImpl works. So, this patch completely side-steps the problem
+        by creating a new version of Structure::get, called
+        Structure::getWithoutMaterializing, which may choose to do an O(n) search if
+        necessary to avoid materialization. I believe this should be fine - the DFG
+        does't call into these code path often enough for this to matter, and most of
+        the time, the Structure that we call this on will already have a property
+        table because some inline cache would have already called ::get() on that
+        Structure.
+        
+        Also cleaned up the materialization logic: we can stop the search as soon as
+        we find any Structure with a property table rather than searching all the way
+        for a pinned one.
+
+        * bytecode/GetByIdStatus.cpp:
+        (JSC::GetByIdStatus::computeFor):
+        * bytecode/PutByIdStatus.cpp:
+        (JSC::PutByIdStatus::computeFromLLInt):
+        (JSC::PutByIdStatus::computeFor):
+        * runtime/Structure.cpp:
+        (JSC::Structure::findStructuresAndMapForMaterialization):
+        (JSC::Structure::materializePropertyMap):
+        (JSC::Structure::getWithoutMaterializing):
+        (JSC):
+        * runtime/Structure.h:
+        (Structure):
+        * runtime/StructureInlines.h:
+        (JSC::Structure::getWithoutMaterializing):
+        (JSC):
+
 2013-07-15  Oliver Hunt  <oliver@apple.com>
 
         Merged dfgFourthTier r148047
index db4aa9b..78eca45 100644 (file)
@@ -164,7 +164,7 @@ GetByIdStatus GetByIdStatus::computeFor(CodeBlock* profiledBlock, unsigned bytec
         Structure* structure = stubInfo.u.getByIdSelf.baseObjectStructure.get();
         unsigned attributesIgnored;
         JSCell* specificValue;
-        result.m_offset = structure->get(
+        result.m_offset = structure->getWithoutMaterializing(
             *profiledBlock->vm(), ident, attributesIgnored, specificValue);
         if (structure->isDictionary())
             specificValue = 0;
@@ -189,7 +189,7 @@ GetByIdStatus GetByIdStatus::computeFor(CodeBlock* profiledBlock, unsigned bytec
             
             unsigned attributesIgnored;
             JSCell* specificValue;
-            PropertyOffset myOffset = structure->get(
+            PropertyOffset myOffset = structure->getWithoutMaterializing(
                 *profiledBlock->vm(), ident, attributesIgnored, specificValue);
             if (structure->isDictionary())
                 specificValue = 0;
@@ -274,7 +274,7 @@ GetByIdStatus GetByIdStatus::computeFor(VM& vm, Structure* structure, Identifier
     result.m_wasSeenInJIT = false; // To my knowledge nobody that uses computeFor(VM&, Structure*, Identifier&) reads this field, but I might as well be honest: no, it wasn't seen in the JIT, since I computed it statically.
     unsigned attributes;
     JSCell* specificValue;
-    result.m_offset = structure->get(vm, ident, attributes, specificValue);
+    result.m_offset = structure->getWithoutMaterializing(vm, ident, attributes, specificValue);
     if (!isValidOffset(result.m_offset))
         return GetByIdStatus(TakesSlowPath); // It's probably a prototype lookup. Give up on life for now, even though we could totally be way smarter about it.
     if (attributes & Accessor)
index 24a57eb..48a3bc0 100644 (file)
@@ -49,7 +49,7 @@ PutByIdStatus PutByIdStatus::computeFromLLInt(CodeBlock* profiledBlock, unsigned
     
     if (instruction[0].u.opcode == LLInt::getOpcode(llint_op_put_by_id)
         || instruction[0].u.opcode == LLInt::getOpcode(llint_op_put_by_id_out_of_line)) {
-        PropertyOffset offset = structure->get(*profiledBlock->vm(), ident);
+        PropertyOffset offset = structure->getWithoutMaterializing(*profiledBlock->vm(), ident);
         if (!isValidOffset(offset))
             return PutByIdStatus(NoInformation, 0, 0, 0, invalidOffset);
         
@@ -68,7 +68,7 @@ PutByIdStatus PutByIdStatus::computeFromLLInt(CodeBlock* profiledBlock, unsigned
     ASSERT(newStructure);
     ASSERT(chain);
     
-    PropertyOffset offset = newStructure->get(*profiledBlock->vm(), ident);
+    PropertyOffset offset = newStructure->getWithoutMaterializing(*profiledBlock->vm(), ident);
     if (!isValidOffset(offset))
         return PutByIdStatus(NoInformation, 0, 0, 0, invalidOffset);
     
@@ -103,8 +103,9 @@ PutByIdStatus PutByIdStatus::computeFor(CodeBlock* profiledBlock, unsigned bytec
         return PutByIdStatus(TakesSlowPath, 0, 0, 0, invalidOffset);
         
     case access_put_by_id_replace: {
-        PropertyOffset offset = stubInfo.u.putByIdReplace.baseObjectStructure->get(
-            *profiledBlock->vm(), ident);
+        PropertyOffset offset =
+            stubInfo.u.putByIdReplace.baseObjectStructure->getWithoutMaterializing(
+                *profiledBlock->vm(), ident);
         if (isValidOffset(offset)) {
             return PutByIdStatus(
                 SimpleReplace,
@@ -118,8 +119,9 @@ PutByIdStatus PutByIdStatus::computeFor(CodeBlock* profiledBlock, unsigned bytec
     case access_put_by_id_transition_normal:
     case access_put_by_id_transition_direct: {
         ASSERT(stubInfo.u.putByIdTransition.previousStructure->transitionWatchpointSetHasBeenInvalidated());
-        PropertyOffset offset = stubInfo.u.putByIdTransition.structure->get(
-            *profiledBlock->vm(), ident);
+        PropertyOffset offset = 
+            stubInfo.u.putByIdTransition.structure->getWithoutMaterializing(
+                *profiledBlock->vm(), ident);
         if (isValidOffset(offset)) {
             return PutByIdStatus(
                 SimpleTransition,
@@ -152,7 +154,8 @@ PutByIdStatus PutByIdStatus::computeFor(VM& vm, JSGlobalObject* globalObject, St
     
     unsigned attributes;
     JSCell* specificValue;
-    PropertyOffset offset = structure->get(vm, ident, attributes, specificValue);
+    PropertyOffset offset = structure->getWithoutMaterializing(
+        vm, ident, attributes, specificValue);
     if (isValidOffset(offset)) {
         if (attributes & (Accessor | ReadOnly))
             return PutByIdStatus(TakesSlowPath);
index f551eae..e365dff 100644 (file)
@@ -236,34 +236,38 @@ void Structure::destroy(JSCell* cell)
     static_cast<Structure*>(cell)->Structure::~Structure();
 }
 
-void Structure::materializePropertyMap(VM& vm)
+void Structure::findStructuresAndMapForMaterialization(Vector<Structure*, 8>& structures, PropertyTable*& table)
 {
-    ASSERT(structure()->classInfo() == &s_info);
-    ASSERT(!propertyTable());
-
-    Vector<Structure*, 8> structures;
-    structures.append(this);
+    ASSERT(structures.isEmpty());
+    table = 0;
 
-    Structure* structure = this;
-
-    // Search for the last Structure with a property table.
-    while ((structure = structure->previousID())) {
-        if (structure->m_isPinnedPropertyTable) {
-            ASSERT(structure->propertyTable());
-            ASSERT(!structure->previousID());
-
-            propertyTable().set(vm, this, structure->propertyTable()->copy(vm, 0, numberOfSlotsForLastOffset(m_offset, m_inlineCapacity)));
-            break;
+    for (Structure* structure = this; structure; structure = structure->previousID()) {
+        if (structure->propertyTable()) {
+            table = structure->propertyTable().get();
+            return;
         }
-
+        
         structures.append(structure);
     }
+}
 
-    if (!propertyTable())
+void Structure::materializePropertyMap(VM& vm)
+{
+    ASSERT(structure()->classInfo() == &s_info);
+    ASSERT(!propertyTable());
+
+    Vector<Structure*, 8> structures;
+    PropertyTable* table;
+    
+    findStructuresAndMapForMaterialization(structures, table);
+    
+    if (!table)
         createPropertyMap(vm, numberOfSlotsForLastOffset(m_offset, m_inlineCapacity));
+    else
+        propertyTable().set(vm, this, table->copy(vm, 0, numberOfSlotsForLastOffset(m_offset, m_inlineCapacity)));
 
-    for (ptrdiff_t i = structures.size() - 1; i >= 0; --i) {
-        structure = structures[i];
+    for (size_t i = structures.size(); i--;) {
+        Structure* structure = structures[i];
         if (!structure->m_nameInPrevious)
             continue;
         PropertyMapEntry entry(vm, this, structure->m_nameInPrevious.get(), structure->m_offset, structure->m_attributesInPrevious, structure->m_specificValueInPrevious.get());
@@ -743,6 +747,35 @@ PropertyTable* Structure::copyPropertyTableForPinning(VM& vm, Structure* owner)
     return PropertyTable::create(vm, numberOfSlotsForLastOffset(m_offset, m_inlineCapacity));
 }
 
+PropertyOffset Structure::getWithoutMaterializing(VM&, PropertyName propertyName, unsigned& attributes, JSCell*& specificValue)
+{
+    Vector<Structure*, 8> structures;
+    PropertyTable* table;
+    
+    findStructuresAndMapForMaterialization(structures, table);
+    
+    if (table) {
+        PropertyMapEntry* entry = table->find(propertyName.uid()).first;
+        if (entry) {
+            attributes = entry->attributes;
+            specificValue = entry->specificValue.get();
+            return entry->offset;
+        }
+    }
+    
+    for (unsigned i = structures.size(); i--;) {
+        Structure* structure = structures[i];
+        if (structure->m_nameInPrevious.get() != propertyName.uid())
+            continue;
+        
+        attributes = structure->m_attributesInPrevious;
+        specificValue = structure->m_specificValueInPrevious.get();
+        return structure->m_offset;
+    }
+    
+    return invalidOffset;
+}
+
 PropertyOffset Structure::get(VM& vm, PropertyName propertyName, unsigned& attributes, JSCell*& specificValue)
 {
     ASSERT(structure()->classInfo() == &s_info);
index 45379ef..adcad2a 100644 (file)
@@ -235,6 +235,9 @@ public:
     PropertyOffset get(VM&, const WTF::String& name);
     JS_EXPORT_PRIVATE PropertyOffset get(VM&, PropertyName, unsigned& attributes, JSCell*& specificValue);
 
+    PropertyOffset getWithoutMaterializing(VM&, PropertyName);
+    PropertyOffset getWithoutMaterializing(VM&, PropertyName, unsigned& attributes, JSCell*& specificValue);
+
     bool hasGetterSetterProperties() const { return m_hasGetterSetterProperties; }
     bool hasReadOnlyOrGetterSetterPropertiesExcludingProto() const { return m_hasReadOnlyOrGetterSetterPropertiesExcludingProto; }
     void setHasGetterSetterProperties(bool is__proto__)
@@ -353,6 +356,8 @@ private:
 
     static Structure* create(VM&, const Structure*);
         
+    void findStructuresAndMapForMaterialization(Vector<Structure*, 8>& structures, PropertyTable*&);
+    
     typedef enum { 
         NoneDictionaryKind = 0,
         CachedDictionaryKind = 1,
index 75ca40d..49a861c 100644 (file)
@@ -78,6 +78,14 @@ inline PropertyOffset Structure::get(VM& vm, const WTF::String& name)
     return entry ? entry->offset : invalidOffset;
 }
     
+inline PropertyOffset Structure::getWithoutMaterializing(VM& vm, PropertyName propertyName)
+{
+    unsigned attributesIgnored;
+    JSCell* specificValueIgnored;
+    return getWithoutMaterializing(
+        vm, propertyName, attributesIgnored, specificValueIgnored);
+}
+
 inline bool Structure::masqueradesAsUndefined(JSGlobalObject* lexicalGlobalObject)
 {
     return typeInfo().masqueradesAsUndefined() && globalObject() == lexicalGlobalObject;