Script streaming: Add an option to make the main thread block (wait for parsing)

Source/bindings/core/v8/ScriptStreamer.cpp

Index: Source/bindings/core/v8/ScriptStreamer.cpp
diff --git a/Source/bindings/core/v8/ScriptStreamer.cpp b/Source/bindings/core/v8/ScriptStreamer.cpp
index efbe8a5cba741a17b9c1f5df61ce4d09c83f8a4a..517dcb7013ff47540e799d9094ff68a1e905c7bc 100644
--- a/Source/bindings/core/v8/ScriptStreamer.cpp
+++ b/Source/bindings/core/v8/ScriptStreamer.cpp
@@ -13,6 +13,7 @@
 #include "core/fetch/ScriptResource.h"
 #include "core/frame/Settings.h"
 #include "platform/SharedBuffer.h"
+#include "platform/TraceEvent.h"
 #include "public/platform/Platform.h"
 #include "wtf/MainThread.h"
 #include "wtf/text/TextEncodingRegistry.h"
@@ -224,28 +225,20 @@ void ScriptStreamer::startStreaming(PendingScript& script, Settings* settings, S
         blink::Platform::current()->histogramEnumeration(startedStreamingHistogramName(scriptType), 0, 2);
 }
 
-void ScriptStreamer::streamingComplete()
+void ScriptStreamer::streamingCompleteOnBackgroundThread()
 {
-    ASSERT(isMainThread());
-    // It's possible that the corresponding Resource was deleted before V8
-    // finished streaming. In that case, the data or the notification is not
-    // needed. In addition, if the streaming is suppressed, the non-streaming
-    // code path will resume after the resource has loaded, before the
-    // background task finishes.
-    if (m_detached || m_streamingSuppressed) {
-        deref();
-        return;
-    }
-
-    // We have now streamed the whole script to V8 and it has parsed the
-    // script. We're ready for the next step: compiling and executing the
-    // script.
+    ASSERT(!isMainThread());
+    MutexLocker locker(m_mutex);

[haraken, 2014/10/23 15:32:09]

Let's add ASSERT(!isFinished()).

[marja, 2014/10/24 13:04:41]

I don't think that's true; the streaming can still be suppressed (if we started
the parsing task but then got the code cache).

But, oops, the code doesn't handle that case gracefully. Fixing that. If the
streaming is suppressed, the main thread won't wait and it shouldn't deref()
either because it's too early (the task is still running). Fixed that. So, now
do the cleanup here.

The decision of whether to do the cleanup in the main thread or here was very
subtle - way too subtle. And error prone.

So I made it explicit: if the main thread is waiting right now, then let the
main thread do the cleanup afterwards, otherwise do the cleanup here. I added an
extra variable to track exactly that, so we don't need this "if (foo && bar &&
(!baz || quux)" mess for deciding.

     m_parsingFinished = true;
-
-    notifyFinishedToClient();
-
-    // The background thread no longer holds an implicit reference.
-    deref();
+    if (shouldBlockMainThread()) {
+        // Normally, the main thread is waiting at this point, but it can also
+        // happen that the load is not yet finished (e.g., a parse error). In
+        // that case, notifyFinished will be called eventually and it will not
+        // wait on m_parsingFinishedCondition.
+        m_parsingFinishedCondition.signal();
+    } else {
+        callOnMainThread(WTF::bind(&ScriptStreamer::streamingComplete, this));
+    }
 }
 
 void ScriptStreamer::cancel()
@@ -262,6 +255,7 @@ void ScriptStreamer::cancel()
 
 void ScriptStreamer::suppressStreaming()
 {
+    MutexLocker locker(m_mutex);

[haraken, 2014/10/23 15:32:09]

This lock is a bit too wide. Shall we add the MutexLocker into isFinished() and
use ASSERT(!isFinished()) here?

[marja, 2014/10/24 13:04:41]

Hmm, in notifyFinished() I want to call isFinished and
m_parsingFinishedCondition.wait without releasing the mutex inbetween. (If I
release it, the background thread can go and do m_parsingFinished = true;
m_parsingFinishedCondition.signal(); before the main thread has a chance to
wait().

IMO, not worth micro-optimizing here.

     ASSERT(!m_parsingFinished);
     ASSERT(!m_loadingFinished);
     m_streamingSuppressed = true;
@@ -294,7 +288,8 @@ void ScriptStreamer::notifyAppendData(ScriptResource* resource)
         ASSERT(m_task);
         // ScriptStreamer needs to stay alive as long as the background task is
         // running. This is taken care of with a manual ref() & deref() pair;
-        // the corresponding deref() is in streamingComplete.
+        // the corresponding deref() is in streamingComplete or in
+        // notifyFinished.
         ref();
         ScriptStreamingTask* task = new ScriptStreamingTask(m_task.release(), this);
         ScriptStreamerThread::shared()->postTask(task);
@@ -318,10 +313,33 @@ void ScriptStreamer::notifyFinished(Resource* resource)
     }
     m_stream->didFinishLoading();
     m_loadingFinished = true;
+
+    if (shouldBlockMainThread()) {
+        // Make the main thead wait until the streaming is complete, to make
+        // sure that the script gets the main thread's attention as early as
+        // possible (for possible compiling, if the client wants to do it
+        // right away). Note that blocking here is not any worse than the
+        // non-streaming code path where the main thread eventually blocks
+        // to parse the script.
+        TRACE_EVENT0("v8", "v8.mainThreadWaitingForParserThread");
+        MutexLocker locker(m_mutex);
+        if (!isFinished()) {
+            m_parsingFinishedCondition.wait(m_mutex);
+        }
+        ASSERT(isFinished());
+    }
+
     notifyFinishedToClient();
+
+    if (shouldBlockMainThread() && m_parsingFinished) {

[haraken, 2014/10/23 15:32:09]

Do we need the '&& m_parsingFinished' check? I guess it is always true.

[marja, 2014/10/24 13:04:40]

No, it's false when we didn't even start the parsing task (e.g., if the script
was too small). The comment tried to explain it :)

However, this is too subtle, made it explicit.

+        // streamingComplete won't be called, so do the ramp-down work
+        // here. Since m_parsingFinished is true, we know that there was a
+        // background task and we need to deref().
+        deref();
+    }
 }
 
-ScriptStreamer::ScriptStreamer(ScriptResource* resource, v8::ScriptCompiler::StreamedSource::Encoding encoding, PendingScript::Type scriptType)
+ScriptStreamer::ScriptStreamer(ScriptResource* resource, v8::ScriptCompiler::StreamedSource::Encoding encoding, PendingScript::Type scriptType, ScriptStreamingMode mode)
     : m_resource(resource)
     , m_detached(false)
     , m_stream(new SourceStream(this))
@@ -332,9 +350,37 @@ ScriptStreamer::ScriptStreamer(ScriptResource* resource, v8::ScriptCompiler::Str
     , m_haveEnoughDataForStreaming(false)
     , m_streamingSuppressed(false)
     , m_scriptType(scriptType)
+    , m_scriptStreamingMode(mode)
 {
 }
 
+void ScriptStreamer::streamingComplete()
+{
+    // The background task is completed; do the necessary ramp-down in the main
+    // thread.
+    ASSERT(isMainThread());
+    // In the blocking mode, the ramp-down is done in notifyFinished.
+    ASSERT(!shouldBlockMainThread());
+
+    // It's possible that the corresponding Resource was deleted before V8
+    // finished streaming. In that case, the data or the notification is not
+    // needed. In addition, if the streaming is suppressed, the non-streaming
+    // code path will resume after the resource has loaded, before the
+    // background task finishes.
+    if (m_detached || m_streamingSuppressed) {
+        deref();
+        return;
+    }
+
+    // We have now streamed the whole script to V8 and it has parsed the
+    // script. We're ready for the next step: compiling and executing the
+    // script.
+    notifyFinishedToClient();
+
+    // The background thread no longer holds an implicit reference.
+    deref();
+}
+
 void ScriptStreamer::notifyFinishedToClient()
 {
     ASSERT(isMainThread());
@@ -346,6 +392,7 @@ void ScriptStreamer::notifyFinishedToClient()
     // function calling notifyFinishedToClient was already scheduled in the task
     // queue and the upper layer decided that it's not interested in the script
     // and called removeClient.
+    MutexLocker locker(m_mutex);

[haraken, 2014/10/23 15:32:09]

This lock is too wide. We don't need to hold a lock while calling
notifyFinished(). We can add the MutexLocker to isFinished().

[marja, 2014/10/24 13:04:41]

Made this code release the lock before calling client.

     if (isFinished() && m_client)
         m_client->notifyFinished(m_resource);
 }
@@ -374,6 +421,10 @@ bool ScriptStreamer::startStreamingInternal(PendingScript& script, Settings* set
     ASSERT(isMainThread());
     if (!settings || !settings->v8ScriptStreamingEnabled())
         return false;
+    if (settings->v8ScriptStreamingMode() == ScriptStreamingModeOnlyAsyncAndDefer
+        && scriptType == PendingScript::ParsingBlocking)
+        return false;
+
     ScriptResource* resource = script.resource();
     ASSERT(!resource->isLoaded());
     if (!resource->url().protocolIsInHTTPFamily())
@@ -415,7 +466,7 @@ bool ScriptStreamer::startStreamingInternal(PendingScript& script, Settings* set
     // The Resource might go out of scope if the script is no longer needed. We
     // will soon call PendingScript::setStreamer, which makes the PendingScript
     // notify the ScriptStreamer when it is destroyed.
-    RefPtr<ScriptStreamer> streamer = adoptRef(new ScriptStreamer(resource, encoding, scriptType));
+    RefPtr<ScriptStreamer> streamer = adoptRef(new ScriptStreamer(resource, encoding, scriptType, settings->v8ScriptStreamingMode()));
 
     // Decide what kind of cached data we should produce while streaming. By
     // default, we generate the parser cache for streamed scripts, to emulate