<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[150721] branches/dfgFourthTier/Source/JavaScriptCore</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/150721">150721</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2013-05-26 11:44:36 -0700 (Sun, 26 May 2013)</dd>
</dl>

<h3>Log Message</h3>
<pre>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
&quot;allocations&quot; in cti_optimize are the ones where we inform the GC about extra memory
usage.
        
I had a more comprehensive solution (see the bug, &quot;work in progress&quot; 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):</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#branchesdfgFourthTierSourceJavaScriptCoreChangeLog">branches/dfgFourthTier/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#branchesdfgFourthTierSourceJavaScriptCorejitJITStubscpp">branches/dfgFourthTier/Source/JavaScriptCore/jit/JITStubs.cpp</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="branchesdfgFourthTierSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: branches/dfgFourthTier/Source/JavaScriptCore/ChangeLog (150720 => 150721)</h4>
<pre class="diff"><span>
<span class="info">--- branches/dfgFourthTier/Source/JavaScriptCore/ChangeLog        2013-05-26 17:42:44 UTC (rev 150720)
+++ branches/dfgFourthTier/Source/JavaScriptCore/ChangeLog        2013-05-26 18:44:36 UTC (rev 150721)
</span><span class="lines">@@ -1,3 +1,28 @@
</span><ins>+2013-05-25  Filip Pizlo  &lt;fpizlo@apple.com&gt;
+
+        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
+        &quot;allocations&quot; in cti_optimize are the ones where we inform the GC about extra memory
+        usage.
+        
+        I had a more comprehensive solution (see the bug, &quot;work in progress&quot; 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):
+
</ins><span class="cx"> 2013-05-26  Filip Pizlo  &lt;fpizlo@apple.com&gt;
</span><span class="cx"> 
</span><span class="cx">         Merge trunk r150694.
</span></span></pre></div>
<a id="branchesdfgFourthTierSourceJavaScriptCorejitJITStubscpp"></a>
<div class="modfile"><h4>Modified: branches/dfgFourthTier/Source/JavaScriptCore/jit/JITStubs.cpp (150720 => 150721)</h4>
<pre class="diff"><span>
<span class="info">--- branches/dfgFourthTier/Source/JavaScriptCore/jit/JITStubs.cpp        2013-05-26 17:42:44 UTC (rev 150720)
+++ branches/dfgFourthTier/Source/JavaScriptCore/jit/JITStubs.cpp        2013-05-26 18:44:36 UTC (rev 150721)
</span><span class="lines">@@ -42,6 +42,7 @@
</span><span class="cx"> #include &quot;DFGOSREntry.h&quot;
</span><span class="cx"> #include &quot;DFGWorklist.h&quot;
</span><span class="cx"> #include &quot;Debugger.h&quot;
</span><ins>+#include &quot;DeferGC.h&quot;
</ins><span class="cx"> #include &quot;ExceptionHelpers.h&quot;
</span><span class="cx"> #include &quot;GetterSetter.h&quot;
</span><span class="cx"> #include &quot;Heap.h&quot;
</span><span class="lines">@@ -903,6 +904,26 @@
</span><span class="cx"> {
</span><span class="cx">     STUB_INIT_STACK_FRAME(stackFrame);
</span><span class="cx">     
</span><ins>+    // 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-&gt;heap);
+    
</ins><span class="cx">     CallFrame* callFrame = stackFrame.callFrame;
</span><span class="cx">     CodeBlock* codeBlock = callFrame-&gt;codeBlock();
</span><span class="cx">     unsigned bytecodeIndex = stackFrame.args[0].int32();
</span><span class="lines">@@ -984,15 +1005,10 @@
</span><span class="cx">     
</span><span class="cx">     if (worklistState == DFG::Worklist::Compiled) {
</span><span class="cx">         // If we don't have an optimized replacement but we did just get compiled, then
</span><del>-        // 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.
</del><ins>+        // 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.
</ins><span class="cx">         if (!codeBlock-&gt;hasOptimizedReplacement()) {
</span><span class="cx">             codeBlock-&gt;updateAllPredictions();
</span><span class="cx">             if (Options::verboseOSR())
</span></span></pre>
</div>
</div>

</body>
</html>