Make FunctionRareData allocation thread-safe
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 23 Apr 2015 21:56:23 +0000 (21:56 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 23 Apr 2015 21:56:23 +0000 (21:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=144001

Patch by Basile Clement <basile_clement@apple.com> on 2015-04-23
Reviewed by Mark Lam.

The two things we want to prevent are:

 1. A thread seeing a pointer to a not-yet-fully-created rare data from
    a JSFunction
 2. A thread seeing a pointer to a not-yet-fully-created Structure from
    an ObjectAllocationProfile

For 1., only the JS thread can be creating the rare data (in
runtime/CommonSlowPaths.cpp or in dfg/DFGOperations.cpp), so we don't need to
worry about concurrent writes, and we don't need any fences when *reading* the
rare data from the JS thread. Thus we only need a storeStoreFence between the
rare data creation and assignment to m_rareData in
JSFunction::createAndInitializeRareData() to ensure that when the store to
m_rareData is issued, the rare data has been properly created.

For the DFG compilation threads, the only place they can access the
rare data is through JSFunction::rareData(), and so we only need a
loadLoadFence there to ensure that when we see a non-null pointer in
m_rareData, the pointed object will be seen as a fully created
FunctionRareData.

For 2., the structure is created in
ObjectAllocationProfile::initialize() (which appears to be called only by the
JS thread as well, in bytecode/CodeBlock.cpp and on rare data initialization,
which always happen in the JS thread), and read through
ObjectAllocationProfile::structure() and
ObjectAllocationProfile::inlineCapacity(), so following the same reasoning we
put a storeStoreFence in ObjectAllocationProfile::initialize() and a
loadLoadFence in ObjectAllocationProfile::structure() (and change
ObjectAllocationProfile::inlineCapacity() to go through
ObjectAllocationProfile::structure()).

We don't need a fence in ObjectAllocationProfile::clear() because
clearing the structure is already as atomic as it gets.

Finally, notice that we don't care about the ObjectAllocationProfile's
m_allocator as that is only used by ObjectAllocationProfile::initialize() and
ObjectAllocationProfile::clear() that are always run in the JS thread.
ObjectAllocationProfile::isNull() could cause some trouble, but it is
currently only used in the ObjectAllocationProfile::clear()'s ASSERT in the JS
thread.  Doing isNull()-style pre-checks would be wrong in any other concurrent
thread anyway.

* bytecode/ObjectAllocationProfile.h:
(JSC::ObjectAllocationProfile::initialize):
(JSC::ObjectAllocationProfile::structure):
(JSC::ObjectAllocationProfile::inlineCapacity):
* runtime/JSFunction.cpp:
(JSC::JSFunction::allocateAndInitializeRareData):
* runtime/JSFunction.h:
(JSC::JSFunction::rareData):
(JSC::JSFunction::allocationStructure): Deleted.
This is no longer used, as all the accesses to the ObjectAllocationProfile go through the rare data.

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/ObjectAllocationProfile.h
Source/JavaScriptCore/runtime/JSFunction.cpp
Source/JavaScriptCore/runtime/JSFunction.h

index f8e95d7..d16cd63 100644 (file)
@@ -1,3 +1,65 @@
+2015-04-23  Basile Clement  <basile_clement@apple.com>
+
+        Make FunctionRareData allocation thread-safe
+        https://bugs.webkit.org/show_bug.cgi?id=144001
+
+        Reviewed by Mark Lam.
+
+        The two things we want to prevent are:
+
+         1. A thread seeing a pointer to a not-yet-fully-created rare data from
+            a JSFunction
+         2. A thread seeing a pointer to a not-yet-fully-created Structure from
+            an ObjectAllocationProfile
+
+        For 1., only the JS thread can be creating the rare data (in
+        runtime/CommonSlowPaths.cpp or in dfg/DFGOperations.cpp), so we don't need to
+        worry about concurrent writes, and we don't need any fences when *reading* the
+        rare data from the JS thread. Thus we only need a storeStoreFence between the
+        rare data creation and assignment to m_rareData in
+        JSFunction::createAndInitializeRareData() to ensure that when the store to
+        m_rareData is issued, the rare data has been properly created.
+
+        For the DFG compilation threads, the only place they can access the
+        rare data is through JSFunction::rareData(), and so we only need a
+        loadLoadFence there to ensure that when we see a non-null pointer in
+        m_rareData, the pointed object will be seen as a fully created
+        FunctionRareData.
+
+
+        For 2., the structure is created in
+        ObjectAllocationProfile::initialize() (which appears to be called only by the
+        JS thread as well, in bytecode/CodeBlock.cpp and on rare data initialization,
+        which always happen in the JS thread), and read through
+        ObjectAllocationProfile::structure() and
+        ObjectAllocationProfile::inlineCapacity(), so following the same reasoning we
+        put a storeStoreFence in ObjectAllocationProfile::initialize() and a
+        loadLoadFence in ObjectAllocationProfile::structure() (and change
+        ObjectAllocationProfile::inlineCapacity() to go through
+        ObjectAllocationProfile::structure()).
+
+        We don't need a fence in ObjectAllocationProfile::clear() because
+        clearing the structure is already as atomic as it gets.
+
+        Finally, notice that we don't care about the ObjectAllocationProfile's
+        m_allocator as that is only used by ObjectAllocationProfile::initialize() and
+        ObjectAllocationProfile::clear() that are always run in the JS thread.
+        ObjectAllocationProfile::isNull() could cause some trouble, but it is
+        currently only used in the ObjectAllocationProfile::clear()'s ASSERT in the JS
+        thread.  Doing isNull()-style pre-checks would be wrong in any other concurrent
+        thread anyway.
+
+        * bytecode/ObjectAllocationProfile.h:
+        (JSC::ObjectAllocationProfile::initialize):
+        (JSC::ObjectAllocationProfile::structure):
+        (JSC::ObjectAllocationProfile::inlineCapacity):
+        * runtime/JSFunction.cpp:
+        (JSC::JSFunction::allocateAndInitializeRareData):
+        * runtime/JSFunction.h:
+        (JSC::JSFunction::rareData):
+        (JSC::JSFunction::allocationStructure): Deleted.
+        This is no longer used, as all the accesses to the ObjectAllocationProfile go through the rare data.
+
 2015-04-22  Filip Pizlo  <fpizlo@apple.com>
 
         DFG should insert Phantoms late using BytecodeKills and block-local OSR availability
index 9ce12b1..cb4148e 100644 (file)
@@ -89,13 +89,23 @@ public:
         if (inlineCapacity > JSFinalObject::maxInlineCapacity())
             inlineCapacity = JSFinalObject::maxInlineCapacity();
 
+        Structure* structure = vm.prototypeMap.emptyObjectStructureForPrototype(prototype, inlineCapacity);
+
+        // Ensure that if another thread sees the structure, it will see it properly created
+        WTF::storeStoreFence();
+
         m_allocator = allocator;
-        m_structure.set(vm, owner,
-            vm.prototypeMap.emptyObjectStructureForPrototype(prototype, inlineCapacity));
+        m_structure.set(vm, owner, structure);
     }
 
-    Structure* structure() { return m_structure.get(); }
-    unsigned inlineCapacity() { return m_structure->inlineCapacity(); }
+    Structure* structure()
+    {
+        Structure* structure = m_structure.get();
+        // Ensure that if we see the structure, it has been properly created
+        WTF::loadLoadFence();
+        return structure;
+    }
+    unsigned inlineCapacity() { return structure()->inlineCapacity(); }
 
     void clear()
     {
index 0f07683..526ca03 100644 (file)
@@ -116,6 +116,11 @@ FunctionRareData* JSFunction::allocateAndInitializeRareData(ExecState* exec, siz
     if (!prototype)
         prototype = globalObject()->objectPrototype();
     FunctionRareData* rareData = FunctionRareData::create(vm, prototype, inlineCapacity);
+
+    // A DFG compilation thread may be trying to read the rare data
+    // We want to ensure that it sees it properly allocated
+    WTF::storeStoreFence();
+
     m_rareData.set(vm, this, rareData);
     return m_rareData.get();
 }
index 4c3e29f..86dff12 100644 (file)
@@ -118,14 +118,15 @@ public:
         return m_rareData.get();
     }
 
-    FunctionRareData* rareData() { return m_rareData.get(); }
-
-    Structure* allocationStructure()
+    FunctionRareData* rareData()
     {
-        if (!m_rareData)
-            return nullptr;
+        FunctionRareData* rareData = m_rareData.get();
+
+        // The JS thread may be concurrently creating the rare data
+        // If we see it, we want to ensure it has been properly created
+        WTF::loadLoadFence();
 
-        return m_rareData.get()->allocationStructure();
+        return rareData;
     }
 
     bool isHostOrBuiltinFunction() const;