Commit 9055d143 authored by oliver@apple.com's avatar oliver@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):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@153131 268f45cc-cd09-0410-ab3c-d52691b4dbfc
parent 67e0f33a
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
......
......@@ -66,6 +66,7 @@ void WatchpointSet::notifyWriteSlow()
fireAllWatchpoints();
m_isWatched = false;
m_isInvalidated = true;
WTF::storeStoreFence();
}
void WatchpointSet::fireAllWatchpoints()
......
......@@ -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:
......
......@@ -2639,7 +2639,6 @@ bool ByteCodeParser::parseBlock(unsigned limit)
addStructureTransitionCheck(prototype.asCell());
}
}
WTF::loadLoadFence();
ASSERT(putByIdStatus.oldStructure()->transitionWatchpointSetHasBeenInvalidated());
Node* propertyStorage;
......
......@@ -116,7 +116,6 @@ public:
if (iter == m_firstKnownState.end())
return false;
WTF::loadLoadFence();
return iter->value != set->isStillValid();
}
#endif
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment