Title: [109519] trunk/Source/_javascript_Core
- Revision
- 109519
- Author
- [email protected]
- Date
- 2012-03-02 00:04:19 -0800 (Fri, 02 Mar 2012)
Log Message
DFGCodeBlocks should not trace CodeBlocks that are also going to be traced by
virtue of being in the transitive closure
https://bugs.webkit.org/show_bug.cgi?id=80098
Reviewed by Anders Carlsson.
If DFGCodeBlocks traces a CodeBlock that might also be traced via its owner Executable,
then you might have the visitAggregate() method called concurrently by multiple threads.
This is benign on 64-bit -- visitAggregate() and everything it calls turns out to be
racy and slightly imprecise but not unsound. But on 32-bit, visitAggregate() may crash
due to word tearing in ValueProfile bucket updates inside of computeUpdatedPrediction().
It would seem that the fix is just to have DFGCodeBlocks not trace CodeBlocks that are
not jettisoned. But CodeBlocks may be jettisoned later during the GC, so it must trace
any CodeBlock that it knows to be live by virtue of it being reachable from the stack.
Hence the real fix is to make sure that concurrent calls into CodeBlock::visitAggregate()
don't lead to two threads racing over each other as they clobber state. This patch
achieves this with a simple CAS loop: whichever thread wins the CAS race (which is
trivially linearizable) will get to trace the CodeBlock; all other threads give up and
go home.
Unfortunately there will be no new tests. It's possible to reproduce this maybe 1/10
times by running V8-v6's raytrace repeatedly, using the V8 harness hacked to rerun it
even when it's gotten sufficient counts. But that takes a while - sometimes up to a
minute to get a crash. I have no other reliable repro case.
* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::visitAggregate):
* bytecode/CodeBlock.h:
(DFGData):
* heap/DFGCodeBlocks.cpp:
(JSC::DFGCodeBlocks::clearMarks):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (109518 => 109519)
--- trunk/Source/_javascript_Core/ChangeLog 2012-03-02 08:01:01 UTC (rev 109518)
+++ trunk/Source/_javascript_Core/ChangeLog 2012-03-02 08:04:19 UTC (rev 109519)
@@ -1,5 +1,40 @@
2012-03-01 Filip Pizlo <[email protected]>
+ DFGCodeBlocks should not trace CodeBlocks that are also going to be traced by
+ virtue of being in the transitive closure
+ https://bugs.webkit.org/show_bug.cgi?id=80098
+
+ Reviewed by Anders Carlsson.
+
+ If DFGCodeBlocks traces a CodeBlock that might also be traced via its owner Executable,
+ then you might have the visitAggregate() method called concurrently by multiple threads.
+ This is benign on 64-bit -- visitAggregate() and everything it calls turns out to be
+ racy and slightly imprecise but not unsound. But on 32-bit, visitAggregate() may crash
+ due to word tearing in ValueProfile bucket updates inside of computeUpdatedPrediction().
+
+ It would seem that the fix is just to have DFGCodeBlocks not trace CodeBlocks that are
+ not jettisoned. But CodeBlocks may be jettisoned later during the GC, so it must trace
+ any CodeBlock that it knows to be live by virtue of it being reachable from the stack.
+ Hence the real fix is to make sure that concurrent calls into CodeBlock::visitAggregate()
+ don't lead to two threads racing over each other as they clobber state. This patch
+ achieves this with a simple CAS loop: whichever thread wins the CAS race (which is
+ trivially linearizable) will get to trace the CodeBlock; all other threads give up and
+ go home.
+
+ Unfortunately there will be no new tests. It's possible to reproduce this maybe 1/10
+ times by running V8-v6's raytrace repeatedly, using the V8 harness hacked to rerun it
+ even when it's gotten sufficient counts. But that takes a while - sometimes up to a
+ minute to get a crash. I have no other reliable repro case.
+
+ * bytecode/CodeBlock.cpp:
+ (JSC::CodeBlock::visitAggregate):
+ * bytecode/CodeBlock.h:
+ (DFGData):
+ * heap/DFGCodeBlocks.cpp:
+ (JSC::DFGCodeBlocks::clearMarks):
+
+2012-03-01 Filip Pizlo <[email protected]>
+
The JIT should not crash the entire process just because there is not enough executable
memory, if the LLInt is enabled
https://bugs.webkit.org/show_bug.cgi?id=79962
Modified: trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp (109518 => 109519)
--- trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp 2012-03-02 08:01:01 UTC (rev 109518)
+++ trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp 2012-03-02 08:04:19 UTC (rev 109519)
@@ -1619,6 +1619,32 @@
void CodeBlock::visitAggregate(SlotVisitor& visitor)
{
+#if ENABLE(PARALLEL_GC)
+ if (!!m_dfgData) {
+ // I may be asked to scan myself more than once, and it may even happen concurrently.
+ // To this end, use a CAS loop to check if I've been called already. Only one thread
+ // may proceed past this point - whichever one wins the CAS race.
+ unsigned oldValue;
+ do {
+ oldValue = m_dfgData->visitAggregateHasBeenCalled;
+ if (oldValue) {
+ // Looks like someone else won! Return immediately to ensure that we don't
+ // trace the same CodeBlock concurrently. Doing so is hazardous since we will
+ // be mutating the state of ValueProfiles, which contain JSValues, which can
+ // have word-tearing on 32-bit, leading to awesome timing-dependent crashes
+ // that are nearly impossible to track down.
+
+ // Also note that it must be safe to return early as soon as we see the
+ // value true (well, (unsigned)1), since once a GC thread is in this method
+ // and has won the CAS race (i.e. was responsible for setting the value true)
+ // it will definitely complete the rest of this method before declaring
+ // termination.
+ return;
+ }
+ } while (!WTF::weakCompareAndSwap(&m_dfgData->visitAggregateHasBeenCalled, 0, 1));
+ }
+#endif // ENABLE(PARALLEL_GC)
+
if (!!m_alternative)
m_alternative->visitAggregate(visitor);
Modified: trunk/Source/_javascript_Core/bytecode/CodeBlock.h (109518 => 109519)
--- trunk/Source/_javascript_Core/bytecode/CodeBlock.h 2012-03-02 08:01:01 UTC (rev 109518)
+++ trunk/Source/_javascript_Core/bytecode/CodeBlock.h 2012-03-02 08:04:19 UTC (rev 109519)
@@ -1172,6 +1172,7 @@
bool isJettisoned;
bool livenessHasBeenProved; // Initialized and used on every GC.
bool allTransitionsHaveBeenMarked; // Initialized and used on every GC.
+ unsigned visitAggregateHasBeenCalled; // Unsigned to make it work seamlessly with the broadest set of CAS implementations.
};
OwnPtr<DFGData> m_dfgData;
Modified: trunk/Source/_javascript_Core/heap/DFGCodeBlocks.cpp (109518 => 109519)
--- trunk/Source/_javascript_Core/heap/DFGCodeBlocks.cpp 2012-03-02 08:01:01 UTC (rev 109518)
+++ trunk/Source/_javascript_Core/heap/DFGCodeBlocks.cpp 2012-03-02 08:04:19 UTC (rev 109519)
@@ -67,8 +67,10 @@
void DFGCodeBlocks::clearMarks()
{
- for (HashSet<CodeBlock*>::iterator iter = m_set.begin(); iter != m_set.end(); ++iter)
+ for (HashSet<CodeBlock*>::iterator iter = m_set.begin(); iter != m_set.end(); ++iter) {
(*iter)->m_dfgData->mayBeExecuting = false;
+ (*iter)->m_dfgData->visitAggregateHasBeenCalled = false;
+ }
}
void DFGCodeBlocks::deleteUnmarkedJettisonedCodeBlocks()
_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes
