Title: [150721] branches/dfgFourthTier/Source/_javascript_Core
- Revision
- 150721
- Author
- [email protected]
- Date
- 2013-05-26 11:44:36 -0700 (Sun, 26 May 2013)
Log Message
fourthTier: cti_optimize shouldn't allow GCs to get in the way of it seeing the state of its CodeBlock
https://bugs.webkit.org/show_bug.cgi?id=116748
Reviewed by Geoffrey Garen.
This fixes the following race: an optimized version of our code block could be installed
by the GC just as we return from completeAllReadyPlansForVM(), leading us to believe
that the code block isn't ready yet even though it is. Currently this triggers a
RELEASE_ASSERT. We could remove that assertion, but then this case would lead to the
code in question entering into optimizeAfterWarmUp mode. That seems pretty wasteful.
Fix the bug, and hopefully close the door on these bugs for a while, by wrapping
cti_optimize in a DeferGC. There is little downside to doing so since the only
"allocations" in cti_optimize are the ones where we inform the GC about extra memory
usage.
I had a more comprehensive solution (see the bug, "work in progress" patch) but that
one involved adding *more* raciness to cti_optimize. I decided that was a less good
approach once I came to appreciate the simplicity of just using DeferGC.
* jit/JITStubs.cpp:
(JSC::DEFINE_STUB_FUNCTION):
Modified Paths
Diff
Modified: branches/dfgFourthTier/Source/_javascript_Core/ChangeLog (150720 => 150721)
--- branches/dfgFourthTier/Source/_javascript_Core/ChangeLog 2013-05-26 17:42:44 UTC (rev 150720)
+++ branches/dfgFourthTier/Source/_javascript_Core/ChangeLog 2013-05-26 18:44:36 UTC (rev 150721)
@@ -1,3 +1,28 @@
+2013-05-25 Filip Pizlo <[email protected]>
+
+ fourthTier: cti_optimize shouldn't allow GCs to get in the way of it seeing the state of its CodeBlock
+ https://bugs.webkit.org/show_bug.cgi?id=116748
+
+ Reviewed by Geoffrey Garen.
+
+ This fixes the following race: an optimized version of our code block could be installed
+ by the GC just as we return from completeAllReadyPlansForVM(), leading us to believe
+ that the code block isn't ready yet even though it is. Currently this triggers a
+ RELEASE_ASSERT. We could remove that assertion, but then this case would lead to the
+ code in question entering into optimizeAfterWarmUp mode. That seems pretty wasteful.
+
+ Fix the bug, and hopefully close the door on these bugs for a while, by wrapping
+ cti_optimize in a DeferGC. There is little downside to doing so since the only
+ "allocations" in cti_optimize are the ones where we inform the GC about extra memory
+ usage.
+
+ I had a more comprehensive solution (see the bug, "work in progress" patch) but that
+ one involved adding *more* raciness to cti_optimize. I decided that was a less good
+ approach once I came to appreciate the simplicity of just using DeferGC.
+
+ * jit/JITStubs.cpp:
+ (JSC::DEFINE_STUB_FUNCTION):
+
2013-05-26 Filip Pizlo <[email protected]>
Merge trunk r150694.
Modified: branches/dfgFourthTier/Source/_javascript_Core/jit/JITStubs.cpp (150720 => 150721)
--- branches/dfgFourthTier/Source/_javascript_Core/jit/JITStubs.cpp 2013-05-26 17:42:44 UTC (rev 150720)
+++ branches/dfgFourthTier/Source/_javascript_Core/jit/JITStubs.cpp 2013-05-26 18:44:36 UTC (rev 150721)
@@ -42,6 +42,7 @@
#include "DFGOSREntry.h"
#include "DFGWorklist.h"
#include "Debugger.h"
+#include "DeferGC.h"
#include "ExceptionHelpers.h"
#include "GetterSetter.h"
#include "Heap.h"
@@ -903,6 +904,26 @@
{
STUB_INIT_STACK_FRAME(stackFrame);
+ // Defer GC so that it doesn't run between when we enter into this slow path and
+ // when we figure out the state of our code block. This prevents a number of
+ // awkward reentrancy scenarios, including:
+ //
+ // - The optimized version of our code block being jettisoned by GC right after
+ // we concluded that we wanted to use it.
+ //
+ // - An optimized version of our code block being installed just as we decided
+ // that it wasn't ready yet.
+ //
+ // This still leaves the following: anytime we return from cti_optimize, we may
+ // GC, and the GC may either jettison the optimized version of our code block,
+ // or it may install the optimized version of our code block even though we
+ // concluded that it wasn't ready yet.
+ //
+ // Note that jettisoning won't happen if we already initiated OSR, because in
+ // that case we would have already planted the optimized code block into the JS
+ // stack.
+ DeferGC deferGC(stackFrame.vm->heap);
+
CallFrame* callFrame = stackFrame.callFrame;
CodeBlock* codeBlock = callFrame->codeBlock();
unsigned bytecodeIndex = stackFrame.args[0].int32();
@@ -984,15 +1005,10 @@
if (worklistState == DFG::Worklist::Compiled) {
// If we don't have an optimized replacement but we did just get compiled, then
- // either of the following happened:
- // - The compilation failed or was invalidated, in which case the execution count
- // thresholds have already been set appropriately by
- // CodeBlock::setOptimizationThresholdBasedOnCompilationResult() and we have
- // nothing left to do.
- // - GC ran after DFG::Worklist::completeAllReadyPlansForVM() and jettisoned our
- // code block. Obviously that's unfortunate and we'd rather not have that
- // happen, but it can happen, and if it did then the jettisoning logic will
- // have set our threshold appropriately and we have nothing left to do.
+ // the compilation failed or was invalidated, in which case the execution count
+ // thresholds have already been set appropriately by
+ // CodeBlock::setOptimizationThresholdBasedOnCompilationResult() and we have
+ // nothing left to do.
if (!codeBlock->hasOptimizedReplacement()) {
codeBlock->updateAllPredictions();
if (Options::verboseOSR())
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
