Changeset 190599 in webkit for trunk/Source/JavaScriptCore/dfg/DFGPlan.cpp

Timestamp:

Oct 5, 2015, 6:33:39 PM (10 years ago)

Author:

[email protected]

Message:

GC shouldn't cancel every FTL compilation
​https://bugs.webkit.org/show_bug.cgi?id=149821

Reviewed by Saam Barati.

During one of the CodeBlock GC refactorings, we messed up the GC's compilation cancellation
code. The GC should be able to cancel compilation plans if it determines that the plan will
be DOA. But, prior to this fix, that code was killing every FTL compilation. This happened
because the meaning of CodeBlock::isKnownToBeLiveDuringGC() changed.

It's funny that this didn't show up as a bigger slow-down. Basically, those benchmarks that
GC a lot usually don't rely on good compilation, while those benchmarks that do rely on good
compilation usually don't GC a lot. That's probably why this wasn't super obvious when we
broke it.

This change just changes the cancellation logic so that it only cancels plans if the owning
executable is dead. This is safe; in fact the relevant method would be correct even if it
always returned true. It would also be correct if it always returned false. So, compared to
what we had before we changed isKnownToBeLiveDuringGC(), this new code will cancel fewer
compilations. But, that's better than cancelling every compilation. I've filed a bug and
written a FIXME for investigating ways to resurrect the old behavior:
​https://bugs.webkit.org/show_bug.cgi?id=149823

Nonetheless, this change looks like it might be a 1% speed-up on Octane. It improves earley
and gbemu.

• dfg/DFGPlan.cpp:

(JSC::DFG::Plan::isKnownToBeLiveDuringGC):

File:

• trunk/Source/JavaScriptCore/dfg/DFGPlan.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/dfg/DFGPlan.cpp

@@ -642 +642 @@
     if (stage == Cancelled)
         return false;
+
+    // NOTE: From here on, this method can return anything and still be sound. It's sound to return
+    // false because then we'll just cancel the compilation. It's always sound to do that. It's sound
+    // to return true because then we'll just keep alive everything that the compilation needs to
+    // have live. The only thing you have to worry about is performance. The goal of returning false
+    // is to try to minimize the likelihood that we expend effort compiling something that will be
+    // DOA - that is, the code being compiled is only reachable from the compilation worklist itself
+    // or the code being compiled is going to have weak references to things that are otherwise dead.
+    // If you return false too often, then you'll regress performance because you might be killing a
+    // compilation that wouldn't have been DOA, and so you're losing opportunities to run faster
+    // code. If you return true too often, then you'll regress performance because you might expend
+    // too much effort compiling something that will be DOA.
+    
     if (!Heap::isMarked(codeBlock->ownerExecutable()))
         return false;
-    if (!codeBlock->alternative()->isKnownToBeLiveDuringGC())
-        return false;
-    if (!!profiledDFGCodeBlock && !profiledDFGCodeBlock->isKnownToBeLiveDuringGC())
-        return false;
+
+    // FIXME: We could detect if the alternate CodeBlock or the profiled DFG CodeBlock have
+    // experienced troubles and may be jettisoned.
+    // https://bugs.webkit.org/show_bug.cgi?id=149823
+
     return true;
 }