fourthTier: WatchpointSet should make racy uses easier to reason about
authoroliver@apple.com <oliver@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 Jul 2013 03:59:02 +0000 (03:59 +0000)
committeroliver@apple.com <oliver@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 25 Jul 2013 03:59:02 +0000 (03:59 +0000)
https://bugs.webkit.org/show_bug.cgi?id=115299

Reviewed by Anders Carlsson.

The compiler often does things like:

1c) Observe something that would imply that a WatchpointSet ought to be invalid

2c) Check that it is invalid

The main thread often does things like:

1m) Fire the watchpoint set

2m) Do some other thing that would cause the compiler to assume that the WatchpointSet
ought to be invalid

An example is structure transitions, where (1c) is the compiler noticing that a
put_by_id inline cache is in a transition state, with the source structure being S;
(2c) is the compiler asserting that S's watchpoint set is invalid; (1m) is the main
thread firing S's watchpoint set before it does the first transition away from S; and
(2m) is the main thread caching the put_by_id transition away from S.

This is totally fine, except that (1c) and (2c), and (1m) and (2m) could be reordered.
Probably, in most cases, this ought to do enough things that the main thread probably
already has some fencing. But the compiler thread definitely doesn't have fencing. In
any case, we should play it safe and just have additional fencing in all of the
relevant places.

We already have some idioms to put load-load and store-store fences in the right
places. But this change just makes WatchpointSet take care of this for us, thus
reducing the chances of us getting this wrong.

* bytecode/Watchpoint.cpp:
(JSC::WatchpointSet::notifyWriteSlow):
* bytecode/Watchpoint.h:
(WatchpointSet):
(JSC::WatchpointSet::isStillValid):
(JSC::WatchpointSet::hasBeenInvalidated):
(JSC::InlineWatchpointSet::hasBeenInvalidated):
(JSC::InlineWatchpointSet::notifyWrite):
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::parseBlock):
* dfg/DFGDesiredWatchpoints.h:
(JSC::DFG::GenericDesiredWatchpoints::shouldAssumeMixedState):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/Watchpoint.cpp
Source/JavaScriptCore/bytecode/Watchpoint.h
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Source/JavaScriptCore/dfg/DFGDesiredWatchpoints.h

index 5dfc397..fc0dbcf 100644 (file)
@@ -1,3 +1,52 @@
+2013-04-26  Filip Pizlo  <fpizlo@apple.com>
+
+        fourthTier: WatchpointSet should make racy uses easier to reason about
+        https://bugs.webkit.org/show_bug.cgi?id=115299
+
+        Reviewed by Anders Carlsson.
+        
+        The compiler often does things like:
+
+        1c) Observe something that would imply that a WatchpointSet ought to be invalid
+
+        2c) Check that it is invalid
+
+        The main thread often does things like:
+
+        1m) Fire the watchpoint set
+
+        2m) Do some other thing that would cause the compiler to assume that the WatchpointSet
+        ought to be invalid
+
+        An example is structure transitions, where (1c) is the compiler noticing that a
+        put_by_id inline cache is in a transition state, with the source structure being S;
+        (2c) is the compiler asserting that S's watchpoint set is invalid; (1m) is the main
+        thread firing S's watchpoint set before it does the first transition away from S; and
+        (2m) is the main thread caching the put_by_id transition away from S.
+
+        This is totally fine, except that (1c) and (2c), and (1m) and (2m) could be reordered.
+        Probably, in most cases, this ought to do enough things that the main thread probably
+        already has some fencing. But the compiler thread definitely doesn't have fencing. In
+        any case, we should play it safe and just have additional fencing in all of the
+        relevant places.
+
+        We already have some idioms to put load-load and store-store fences in the right
+        places. But this change just makes WatchpointSet take care of this for us, thus
+        reducing the chances of us getting this wrong.
+
+        * bytecode/Watchpoint.cpp:
+        (JSC::WatchpointSet::notifyWriteSlow):
+        * bytecode/Watchpoint.h:
+        (WatchpointSet):
+        (JSC::WatchpointSet::isStillValid):
+        (JSC::WatchpointSet::hasBeenInvalidated):
+        (JSC::InlineWatchpointSet::hasBeenInvalidated):
+        (JSC::InlineWatchpointSet::notifyWrite):
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::parseBlock):
+        * dfg/DFGDesiredWatchpoints.h:
+        (JSC::DFG::GenericDesiredWatchpoints::shouldAssumeMixedState):
+
 2013-07-16  Oliver Hunt <oliver@apple.com>
 
         Merge dfgFourthTier r149233
index d0c17ef..2862cf8 100644 (file)
@@ -66,6 +66,7 @@ void WatchpointSet::notifyWriteSlow()
     fireAllWatchpoints();
     m_isWatched = false;
     m_isInvalidated = true;
+    WTF::storeStoreFence();
 }
 
 void WatchpointSet::fireAllWatchpoints()
index 2054bf7..04663dd 100644 (file)
@@ -56,10 +56,17 @@ public:
     
     // It is safe to call this from another thread.  It may return true
     // even if the set actually had been invalidated, but that ought to happen
-    // only in the case of races, and should be rare.
-    bool isStillValid() const { return !m_isInvalidated; }
+    // only in the case of races, and should be rare. Guarantees that if you
+    // call this after observing something that must imply that the set is
+    // invalidated, then you will see this return false. This is ensured by
+    // issuing a load-load fence prior to querying the state.
+    bool isStillValid() const
+    {
+        WTF::loadLoadFence();
+        return !m_isInvalidated;
+    }
     // Like isStillValid(), may be called from another thread.
-    bool hasBeenInvalidated() const { return m_isInvalidated; }
+    bool hasBeenInvalidated() const { return !isStillValid(); }
     
     // As a convenience, this will ignore 0. That's because code paths in the DFG
     // that create speculation watchpoints may choose to bail out if speculation
@@ -133,6 +140,7 @@ public:
     // only in the case of races, and should be rare.
     bool hasBeenInvalidated() const
     {
+        WTF::loadLoadFence();
         uintptr_t data = m_data;
         if (isFat(data)) {
             WTF::loadLoadFence();
@@ -167,6 +175,7 @@ public:
         if (!(m_data & IsWatchedFlag))
             return;
         m_data |= IsInvalidatedFlag;
+        WTF::storeStoreFence();
     }
     
 private:
index aa7503f..674a1f5 100644 (file)
@@ -2639,7 +2639,6 @@ bool ByteCodeParser::parseBlock(unsigned limit)
                         addStructureTransitionCheck(prototype.asCell());
                     }
                 }
-                WTF::loadLoadFence();
                 ASSERT(putByIdStatus.oldStructure()->transitionWatchpointSetHasBeenInvalidated());
                 
                 Node* propertyStorage;
index cf3d939..c614b3a 100644 (file)
@@ -116,7 +116,6 @@ public:
         if (iter == m_firstKnownState.end())
             return false;
         
-        WTF::loadLoadFence();
         return iter->value != set->isStillValid();
     }
 #endif