Title: [153131] trunk/Source/_javascript_Core
- Revision
- 153131
- Author
- [email protected]
- Date
- 2013-07-24 20:59:02 -0700 (Wed, 24 Jul 2013)
Log Message
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):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (153130 => 153131)
--- trunk/Source/_javascript_Core/ChangeLog 2013-07-25 03:59:00 UTC (rev 153130)
+++ trunk/Source/_javascript_Core/ChangeLog 2013-07-25 03:59:02 UTC (rev 153131)
@@ -1,3 +1,52 @@
+2013-04-26 Filip Pizlo <[email protected]>
+
+ 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 <[email protected]>
Merge dfgFourthTier r149233
Modified: trunk/Source/_javascript_Core/bytecode/Watchpoint.cpp (153130 => 153131)
--- trunk/Source/_javascript_Core/bytecode/Watchpoint.cpp 2013-07-25 03:59:00 UTC (rev 153130)
+++ trunk/Source/_javascript_Core/bytecode/Watchpoint.cpp 2013-07-25 03:59:02 UTC (rev 153131)
@@ -66,6 +66,7 @@
fireAllWatchpoints();
m_isWatched = false;
m_isInvalidated = true;
+ WTF::storeStoreFence();
}
void WatchpointSet::fireAllWatchpoints()
Modified: trunk/Source/_javascript_Core/bytecode/Watchpoint.h (153130 => 153131)
--- trunk/Source/_javascript_Core/bytecode/Watchpoint.h 2013-07-25 03:59:00 UTC (rev 153130)
+++ trunk/Source/_javascript_Core/bytecode/Watchpoint.h 2013-07-25 03:59:02 UTC (rev 153131)
@@ -56,10 +56,17 @@
// 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 @@
// only in the case of races, and should be rare.
bool hasBeenInvalidated() const
{
+ WTF::loadLoadFence();
uintptr_t data = ""
if (isFat(data)) {
WTF::loadLoadFence();
@@ -167,6 +175,7 @@
if (!(m_data & IsWatchedFlag))
return;
m_data |= IsInvalidatedFlag;
+ WTF::storeStoreFence();
}
private:
Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (153130 => 153131)
--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2013-07-25 03:59:00 UTC (rev 153130)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2013-07-25 03:59:02 UTC (rev 153131)
@@ -2639,7 +2639,6 @@
addStructureTransitionCheck(prototype.asCell());
}
}
- WTF::loadLoadFence();
ASSERT(putByIdStatus.oldStructure()->transitionWatchpointSetHasBeenInvalidated());
Node* propertyStorage;
Modified: trunk/Source/_javascript_Core/dfg/DFGDesiredWatchpoints.h (153130 => 153131)
--- trunk/Source/_javascript_Core/dfg/DFGDesiredWatchpoints.h 2013-07-25 03:59:00 UTC (rev 153130)
+++ trunk/Source/_javascript_Core/dfg/DFGDesiredWatchpoints.h 2013-07-25 03:59:02 UTC (rev 153131)
@@ -116,7 +116,6 @@
if (iter == m_firstKnownState.end())
return false;
- WTF::loadLoadFence();
return iter->value != set->isStillValid();
}
#endif
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
