Changeset 249247 in webkit

Timestamp:

Aug 28, 2019 11:49:28 PM (5 years ago)

Author:

mark.lam@apple.com

Message:

DFG/FTL: We should prefetch structures and do a loadLoadFence before doing PrototypeChainIsSane checks.
​https://bugs.webkit.org/show_bug.cgi?id=201281
<rdar://problem/54028228>

Reviewed by Yusuke Suzuki and Saam Barati.

JSTests:

• stress/structure-storedPrototype-should-only-assert-on-the-mutator-thread.js: Added.

Source/JavaScriptCore:

This (see title above) is already the preferred idiom used in most places in our
compiler, except for 2: DFG's SpeculativeJIT::compileGetByValOnString() and FTL's
compileStringCharAt(). Consider the following:

bool prototypeChainIsSane = false;
if (globalObject->stringPrototypeChainIsSane()) {

...
m_graph.registerAndWatchStructureTransition(globalObject->stringPrototype()->structure(vm()));
m_graph.registerAndWatchStructureTransition(globalObject->objectPrototype()->structure(vm()));

prototypeChainIsSane = globalObject->stringPrototypeChainIsSane();

}

What's essential for correctness here is that the stringPrototype and objectPrototype
structures be loaded before the loads in the second stringPrototypeChainIsSane()
check. Without a loadLoadFence before the second stringPrototypeChainIsSane()
check, we can't guarantee that. Elsewhere in the compiler, the preferred idiom
for doing this right is to pre-load the structures first, do a loadLoadFence, and
then do the IsSane check just once after e.g.

Structure* arrayPrototypeStructure = globalObject->arrayPrototype()->structure(m_vm);
Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(m_vm);

if (arrayPrototypeStructure->transitionWatchpointSetIsStillValid() has loadLoadFences.

&& objectPrototypeStructure->transitionWatchpointSetIsStillValid() has loadLoadFences.
&& globalObject->arrayPrototypeChainIsSane()) {

m_graph.registerAndWatchStructureTransition(arrayPrototypeStructure);
m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
...

}

This patch changes DFG's SpeculativeJIT::compileGetByValOnString() and FTL's
compileStringCharAt() to follow the same idiom.

We also fix a bad assertion in Structure::storedPrototype() and
Structure::storedPrototypeObject(). The assertion is only correct when those
methods are called from the mutator thread. The assertion has been updated to
only check its test condition if the current thread is the mutator thread.

• dfg/DFGSpeculativeJIT.cpp:

(JSC::DFG::SpeculativeJIT::compileGetByValOnString):

• ftl/FTLLowerDFGToB3.cpp:

(JSC::FTL::DFG::LowerDFGToB3::compileStringCharAt):

• runtime/StructureInlines.h:

(JSC::Structure::storedPrototype const):
(JSC::Structure::storedPrototypeObject const):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/structure-storedPrototype-should-only-assert-on-the-mutator-thread.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp (modified) (2 diffs)
• Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp (modified) (2 diffs)
• Source/JavaScriptCore/runtime/StructureInlines.h (modified) (2 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2019-08-28  Mark Lam  <mark.lam@apple.com>
+
+        DFG/FTL: We should prefetch structures and do a loadLoadFence before doing PrototypeChainIsSane checks.
+        https://bugs.webkit.org/show_bug.cgi?id=201281
+        <rdar://problem/54028228>
+
+        Reviewed by Yusuke Suzuki and Saam Barati.
+
+        * stress/structure-storedPrototype-should-only-assert-on-the-mutator-thread.js: Added.
+
 2019-08-28  Mark Lam  <mark.lam@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-08-28  Mark Lam  <mark.lam@apple.com>
+
+        DFG/FTL: We should prefetch structures and do a loadLoadFence before doing PrototypeChainIsSane checks.
+        https://bugs.webkit.org/show_bug.cgi?id=201281
+        <rdar://problem/54028228>
+
+        Reviewed by Yusuke Suzuki and Saam Barati.
+
+        This (see title above) is already the preferred idiom used in most places in our
+        compiler, except for 2: DFG's SpeculativeJIT::compileGetByValOnString() and FTL's
+        compileStringCharAt().  Consider the following:
+
+            bool prototypeChainIsSane = false;
+            if (globalObject->stringPrototypeChainIsSane()) {
+                ...
+                m_graph.registerAndWatchStructureTransition(globalObject->stringPrototype()->structure(vm()));
+                m_graph.registerAndWatchStructureTransition(globalObject->objectPrototype()->structure(vm()));
+
+                prototypeChainIsSane = globalObject->stringPrototypeChainIsSane();
+            }
+
+        What's essential for correctness here is that the stringPrototype and objectPrototype
+        structures be loaded before the loads in the second stringPrototypeChainIsSane()
+        check.  Without a loadLoadFence before the second stringPrototypeChainIsSane()
+        check, we can't guarantee that.  Elsewhere in the compiler, the preferred idiom
+        for doing this right is to pre-load the structures first, do a loadLoadFence, and
+        then do the IsSane check just once after e.g.
+
+            Structure* arrayPrototypeStructure = globalObject->arrayPrototype()->structure(m_vm);
+            Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(m_vm);
+
+            if (arrayPrototypeStructure->transitionWatchpointSetIsStillValid() // has loadLoadFences.
+                && objectPrototypeStructure->transitionWatchpointSetIsStillValid() // has loadLoadFences.
+                && globalObject->arrayPrototypeChainIsSane()) {
+
+                m_graph.registerAndWatchStructureTransition(arrayPrototypeStructure);
+                m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
+                ...
+            }
+
+        This patch changes DFG's SpeculativeJIT::compileGetByValOnString() and FTL's
+        compileStringCharAt() to follow the same idiom.
+
+        We also fix a bad assertion in Structure::storedPrototype() and
+        Structure::storedPrototypeObject().  The assertion is only correct when those
+        methods are called from the mutator thread.  The assertion has been updated to
+        only check its test condition if the current thread is the mutator thread.
+
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileGetByValOnString):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileStringCharAt):
+        * runtime/StructureInlines.h:
+        (JSC::Structure::storedPrototype const):
+        (JSC::Structure::storedPrototypeObject const):
+
 2019-08-28  Mark Lam  <mark.lam@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp

@@ -2232 +2232 @@
 
         JSGlobalObject* globalObject = m_jit.globalObjectFor(node->origin.semantic);
-        bool prototypeChainIsSane = false;
+        Structure* stringPrototypeStructure = globalObject->stringPrototype()->structure(vm);
+        Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(vm);
+        WTF::loadLoadFence();
+
         if (globalObject->stringPrototypeChainIsSane()) {
             // FIXME: This could be captured using a Speculation mode that means "out-of-bounds
@@ -2240 +2243 @@
             // indexed properties either.
             // https://bugs.webkit.org/show_bug.cgi?id=144668
-            m_jit.graph().registerAndWatchStructureTransition(globalObject->stringPrototype()->structure(vm));
-            m_jit.graph().registerAndWatchStructureTransition(globalObject->objectPrototype()->structure(vm));
-            prototypeChainIsSane = globalObject->stringPrototypeChainIsSane();
-        }
-        if (prototypeChainIsSane) {
+            m_jit.graph().registerAndWatchStructureTransition(stringPrototypeStructure);
+            m_jit.graph().registerAndWatchStructureTransition(objectPrototypeStructure);
+
 #if USE(JSVALUE64)
             addSlowPathGenerator(makeUnique<SaneStringGetByValSlowPathGenerator>(

trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

@@ -6979 +6979 @@
         } else {
             JSGlobalObject* globalObject = m_graph.globalObjectFor(m_node->origin.semantic);
-            
-            bool prototypeChainIsSane = false;
+            Structure* stringPrototypeStructure = globalObject->stringPrototype()->structure(vm());
+            Structure* objectPrototypeStructure = globalObject->objectPrototype()->structure(vm());
+            WTF::loadLoadFence();
+
             if (globalObject->stringPrototypeChainIsSane()) {
                 // FIXME: This could be captured using a Speculation mode that means
@@ -6987 +6989 @@
                 // https://bugs.webkit.org/show_bug.cgi?id=144668
                 
-                m_graph.registerAndWatchStructureTransition(globalObject->stringPrototype()->structure(vm()));
-                m_graph.registerAndWatchStructureTransition(globalObject->objectPrototype()->structure(vm()));
-
-                prototypeChainIsSane = globalObject->stringPrototypeChainIsSane();
-            }
-            if (prototypeChainIsSane) {
+                m_graph.registerAndWatchStructureTransition(stringPrototypeStructure);
+                m_graph.registerAndWatchStructureTransition(objectPrototypeStructure);
+
                 LBasicBlock negativeIndex = m_out.newBlock();
                     

trunk/Source/JavaScriptCore/runtime/StructureInlines.h

@@ -109 +109 @@
 ALWAYS_INLINE JSValue Structure::storedPrototype(const JSObject* object) const
 {
-    ASSERT(object->structure() == this);
+    ASSERT(!isMainThread() || object->structure() == this);
     if (hasMonoProto())
         return storedPrototype();
@@ -117 +117 @@
 ALWAYS_INLINE JSObject* Structure::storedPrototypeObject(const JSObject* object) const
 {
-    ASSERT(object->structure() == this);
+    ASSERT(!isMainThread() || object->structure() == this);
     if (hasMonoProto())
         return storedPrototypeObject();