Stream scripts to V8 as they load - Blink side.

Stream scripts to V8 as they load - Blink side.

V8 will parse the script as it's loading.

The V8 side which adds the streaming API is here:

https://codereview.chromium.org/366153002/

BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182048

[haraken, 2014-08-17 16:05:28]

This is an incredibly exciting change! I'm happy to review the Blink side
changes :)

Two main questions about this CL:

- Do you have any performance numbers about how much this change improves PLT?
We'll need the numbers to justify the change.

- How does the streaming parser work with the V8 compilation cache? If I'm
understanding things correctly, the following scenario can happen:

(1) The main thread initiates a streaming parsing. At this point, script data is
not yet loaded and thus the main thread cannot judge if it can use the V8
compilation cache or not. 
(2) A background thread finishes the streaming parsing.
(3) The main thread notices that it can just use code that is already cached in
the V8 compilation cache.

In this case, we'll end up wasting performance/memory/power resources for
unnecessary parsing. I'm curious how we're going to address the issue.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptSourceCode.h (right):

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptSourceCode.h:94: RefPtr<V8ScriptStreamer>
m_streamer;

In my understanding, V8ScriptStreamer is owned by either PendingScript (if the
script is still pending) or ScriptSourceCode (if the script is going to be
compiled). If so, can we just use an OwnPtr for V8ScriptStreamer? Do we need to
use RefPtrs?

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:102:
resource->setCachedMetadata(streamer->cachedDataDataType(),
reinterpret_cast<const char*>(newCachedData->data), newCachedData->length);

Don't you need to call resource->clearCachedMetadata()? See line 111 and 112.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptStreamer.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:2: * Copyright (C) 2014 Google Inc.
All rights reserved.

Nit: Use the 3-line copyright.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:48: class SourceStream : public
v8::ExternalSourceStream {

(I haven't reviewed this class yet. I'll take a closer look after other
high-level issues are resolved.)

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:185: // TODO(marja): Here we can
limit based on the total length, if it

Nit: TODO(marja) => FIXME

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:234: if
(V8ScriptStreamerThread::shared()->isRunningTask()) {

Given that we currently support only parser-blocking scripts, shouldn't this be
an ASSERT at the moment like this?

  ASSERT(!V8ScriptStreamerThread::shared()->isRunningTask());

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:237: // the running task can block
and wait for data from the network.

Just to confirm: Is this the reason it's not easy to support not-parser-blocking
scripts?

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:249: ScriptState* scriptState =
ScriptState::forMainWorld(frame);

Just to confirm: Script streaming is used only in the main world, right?

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:264:
V8ScriptRunner::CacheDecider(0, resource, v8CacheOptions,
&streamer->m_cachedDataDataType, &compileOption, &ignore);

I don't fully understand why you can decide whether you should use V8
compilation cache or not at this point. Without loading the full source code, it
wouldn't be possible to make the decision.

This is one of my main concerns about this CL.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:291: // completed.

Can we create a helper method like notifyFinishedToHTMLScriptRunner()?

void notifyFinishedToHTMLScriptRunner() {
  if (m_v8Done && m_loadingDone)
    m_runner->notifyFinished(m_script->resource());
}

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:291: // completed.

Help me understand: In what scenario is it possible that m_v8Done becomes true
before m_loadingDone becomes true? Does it happen only when the streaming
processing is cancelled explicitly?

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:301: if (refCount() == 1) {

It's fragile to rely on the reference counter to judge if the PendingScript is
gone. Can you avoid using refCount() and instead use a more explicit way to
judge the fact?

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:315: // completed.

Ditto. Use notifyFinishedToHTMLScriptRunner().

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:335: , m_source(m_stream) // Source
takes ownership of m_stream.

Given that StreamedSource has an ownership of the SourceStream, isn't there any
way to get the SourceStream from the StreamedSource?

Then we can remove the m_stream member from V8ScriptStreamer and write the above
code like this.

  m_source(adoptPtr(new SourceStream(script->resource())))

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:335: , m_source(m_stream) // Source
takes ownership of m_stream.

BTW, "StreamedSource" and "SourceStream" are confusing. Can we rename to
something better?

StreamedSource => SourceStreamController ?

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:338: { }

Nit: Write { and } in separate lines.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptStreamer.h (right):

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.h:2: * Copyright (C) 2014 Google Inc.
All rights reserved.

Nit: Use the 3-line copyright.

// Copyright 2014 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.h:47: class V8ScriptStreamer : public
RefCounted<V8ScriptStreamer> {

V8ScriptStreamer => ScriptStreamer

We're planning to drop a "V8" prefix from V8-binding classes, so let's not add
the prefix to newly introduced classes.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.h:47: class V8ScriptStreamer : public
RefCounted<V8ScriptStreamer> {

(We might want to move V8ScriptStreamer to oilpan's heap, but it's OK for now to
use a RefPtr. As far as I see, it looks like that there are a couple of
non-trivial issues in moving V8ScriptStreamer to oilpan's heap.)

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.h:49: static bool
startStreaming(PendingScript*, HTMLScriptRunner*);

Add a comment about what startStreaming() does.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.h:70: V8ScriptStreamer(PendingScript*,
HTMLScriptRunner*);

Why do both V8ScriptStreamer's constructor and startStreaming() need to take the
same parameters? Can't the startStreaming() use the PendingScript and
HTMLScriptRunner that are passed to V8ScriptStreamer's constructor?

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptStreamerThread.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.cpp:2: * Copyright (C) 2014
Google Inc. All rights reserved.

Nit: Use the 3-line copyright.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.cpp:41: ASSERT(!s_sharedThread);

Add ASSERT(isMainThread()).

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.cpp:47: ASSERT(s_sharedThread);

Add ASSERT(isMainThread()).

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.cpp:51: &&
s_sharedThread->isRunningTask()) {

Why is the s_sharedThread->isRunningTask() check needed?

Probably can we completely get rid of the m_runningTask flag?

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.cpp:72: m_runningTask = true;

Add ASSERT(isMainThread()).

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptStreamerThread.h (right):

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.h:2: * Copyright (C) 2014 Google
Inc. All rights reserved.

Nit: Use the 3-line copyright.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.h:40: class
V8ScriptStreamerThread {

V8ScriptStreamerThread => ScriptStreamerThread

We don't want to have a prefix "V8" for newly introduced classes.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.h:75: // at a time. TODO(marja):
Use a thread pool and stream multiple scripts.

Nit: TODO(marja) => FIXME.

https://codereview.chromium.org/368283002/diff/620001/Source/core/Init.cpp
File Source/core/Init.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/core/Init.cpp#ne...
Source/core/Init.cpp:132: V8ScriptStreamerThread::shutdown();

We want to shutdown threads in the reverse order of init() calls. So let's call
V8ScriptStreamerThread::shutdown() and then HTMLParserThread::shutdown().

https://codereview.chromium.org/368283002/diff/620001/Source/core/dom/Pending...
File Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/core/dom/Pending...
Source/core/dom/PendingScript.cpp:38: m_streamer->cancel();

Doing something non-trivial in a destructor is likely to cause an issue in
oilpan builds (although there would be no issue in this particular case at this
point).

Can you:

- define PendingScript::dispose() and call m_streamer->cancel() in the
PendingScript::dispose()
- call PendingScript::dispose() from HTMLScriptRunner::detach()

?

https://codereview.chromium.org/368283002/diff/620001/Source/core/dom/Pending...
File Source/core/dom/PendingScript.h (right):

https://codereview.chromium.org/368283002/diff/620001/Source/core/dom/Pending...
Source/core/dom/PendingScript.h:108: m_streamer =
RefPtr<V8ScriptStreamer>(streamer);

RefPtr<V8ScriptStreamer> wouldn't be needed.

https://codereview.chromium.org/368283002/diff/620001/Source/core/dom/Pending...
Source/core/dom/PendingScript.h:111: PassRefPtr<V8ScriptStreamer>
releaseStreamer() { return m_streamer.release(); }

Is it possible that releaseStreamer() is called before
streamer->notifyFinished() is called? If so, that seems problematic because no
one will call streamer->notifyFinished() after the PendingScript releases the
streamer. If no, can we add an ASSERT about it?

The point is that we want to make sure that notifyFinished is certainly called.
Otherwise, it will cause dead lock.

https://codereview.chromium.org/368283002/diff/620001/Source/core/fetch/Scrip...
File Source/core/fetch/ScriptResource.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/core/fetch/Scrip...
Source/core/fetch/ScriptResource.cpp:52: void
ScriptResource::didAddClient(ResourceClient* c)

Nit: c => client

https://codereview.chromium.org/368283002/diff/620001/Source/core/fetch/Scrip...
Source/core/fetch/ScriptResource.cpp:61:
ResourceClientWalker<ScriptResourceClient> w(m_clients);

Nit: w => walker

https://codereview.chromium.org/368283002/diff/620001/Source/core/fetch/Scrip...
Source/core/fetch/ScriptResource.cpp:62: while (ScriptResourceClient* c =
w.next()) {

Nit: c => client

https://codereview.chromium.org/368283002/diff/620001/Source/core/html/parser...
File Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:78: // streaming).

Move this comment to above line 74.

https://codereview.chromium.org/368283002/diff/620001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:78: // streaming).

It's a bit weird that the lifetime of not-streamed loading is taken care of by
the HTMLScriptRunner whereas the lifetime of streamed loading is taken care of
by PendingScripts. Can we refactor the code so that all loadings are taken care
of by PendingScripts?

(I guess you'll anyway need to make the change when you support
not-parser-blocking scripts.)

https://codereview.chromium.org/368283002/diff/620001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:118: ASSERT(!script.isStreaming());

Help me understand: Why is it guaranteed that script.isStreaming() is false at
this point?

https://codereview.chromium.org/368283002/diff/620001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:203: // when the streaming has
completed too.

This comment is a bit misleading. Slightly better:

// For a parser-blocking script, this function should be called only when the
streaming is complete.

https://codereview.chromium.org/368283002/diff/620001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:354: // TODO(marja): in addition to
parser blocking scripts, stream also

Nit: TODO(marja) => FIXME

https://codereview.chromium.org/368283002/diff/620001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:355: // scripts which are not
parser blocking.

I wonder if there is any particular reason you cannot enable the streaming for
not-parser-blocking scripts. I'm fine with leaving this as a FIXME for a
separate CL, but I'm curious if there is any particular reason you cannot do it
easily.

https://codereview.chromium.org/368283002/diff/620001/Source/core/xml/parser/...
File Source/core/xml/parser/XMLDocumentParser.h (right):

https://codereview.chromium.org/368283002/diff/620001/Source/core/xml/parser/...
Source/core/xml/parser/XMLDocumentParser.h:68: class XMLDocumentParser FINAL :
public ScriptableDocumentParser, public ScriptResourceClient {

Why does XMLDocumentParser need to be a ScriptResourceClient?

[marja, 2014-08-20 11:45:57]

Thanks for comments!

1)

We talked about the code caching over chat. Here are some clarifications for
other people possibly reading this:

- The thing we're talking about is the "code cache" (which is associated to the
Resource), not the CompilationCache on V8 side. The code cache doesn't exist
yet, but vogelheim@ is working on it.

- So the situation that can possibly happen is that 1) we start streaming a
script, we stream and parse it.. 2) the script is fully loaded now 3) when
compiling, we notice that there is a code cache, and we don't actually need to
compile the code. So we have unnecessarily parsed it.

- Luckily, this situation is possible to avoid, since the Resource will notice
when it has cached metadata (see ResourceLoader::didReceiveCachedMetadata and
the code path / IPCs leading to it).

- So at that point, we can suppress the streaming (watch the load but not feed
the data to V8) and when the load has finished, we'll resume the normal
non-streaming compile path.

The latest patch set implements this behavior. I experimented, and noticed that
the "has metadata" notification arrives early during a script load (in my
experiments we noticed it before feeding any data to V8, but it's possible that
we first feed one data chunk and notice it then).

2) Other changes I did:
- Made some ScriptStreamer functions private: those were not part of the API and
only needed to be called from inside the streaming subsystem.
- Cancelling the streaming from the Streamer side (e.g., when the encoding is
unsupported) doesn't make sense now that I think of it - the only sensible way
is to suppress the streaming (that is, watch the load but not give data to V8,
and then notify HTMLScriptRunner when the load is done). So implemented that.
- Reloading scripts didn't work, fixed that.

3) Other comments:

My intention has been to do / comment on all the comments; if I have missed
some, it's by omission, not on purpose.

I don't have performance numbers yet (will work on them next).

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptSourceCode.h (right):

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptSourceCode.h:94: RefPtr<V8ScriptStreamer>
m_streamer;
On 2014/08/17 16:05:25, haraken wrote:
> 
> In my understanding, V8ScriptStreamer is owned by either PendingScript (if the
> script is still pending) or ScriptSourceCode (if the script is going to be
> compiled). If so, can we just use an OwnPtr for V8ScriptStreamer? Do we need
to
> use RefPtrs?

I don't think we can get away without RefPtrs; the main reason is the situation
when we're streaming.

So we have a PendingScript and then we're also streaming the script to V8.
PendingScript can go away any time, but as the V8 side is still accessing the
V8Streamer (or rather, the SourceStream it holds), it must be kept alive. So
PendingScript cannot own it. Actually, nothing on the Blink side can own it at
that point, since basically everything on the Blink side can ramp down and we
still need to give the V8 background thread some time to realize that.

Then again, when the V8 side operations have stopped, the PendingScript can own
the V8Streamer again (if the PendingScript goes away, it can be destructed).

So it's logical that at this is handled with references; both PendingScript and
the V8 side hold a reference to V8ScriptStreamer. (The V8 side reference is now
tracked with explicit ref / deref.)

However, I simplified this so that we don't have
PendingScript::releaseStreamer(), so that PendingScript will also hold a
reference until it's deleted (or set to track a different resource) - it's
simpler that way.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:102:
resource->setCachedMetadata(streamer->cachedDataDataType(),
reinterpret_cast<const char*>(newCachedData->data), newCachedData->length);
On 2014/08/17 16:05:25, haraken wrote:
> 
> Don't you need to call resource->clearCachedMetadata()? See line 111 and 112.

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptStreamer.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:2: * Copyright (C) 2014 Google Inc.
All rights reserved.
On 2014/08/17 16:05:27, haraken wrote:
> 
> Nit: Use the 3-line copyright.

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:48: class SourceStream : public
v8::ExternalSourceStream {
On 2014/08/17 16:05:26, haraken wrote:
> 
> (I haven't reviewed this class yet. I'll take a closer look after other
> high-level issues are resolved.)

Acknowledged.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:185: // TODO(marja): Here we can
limit based on the total length, if it
On 2014/08/17 16:05:27, haraken wrote:
> 
> Nit: TODO(marja) => FIXME

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:234: if
(V8ScriptStreamerThread::shared()->isRunningTask()) {
On 2014/08/17 16:05:26, haraken wrote:
> 
> Given that we currently support only parser-blocking scripts, shouldn't this
be
> an ASSERT at the moment like this?
> 
>   ASSERT(!V8ScriptStreamerThread::shared()->isRunningTask());

It can still be that we're already running a task. Experimentally, I got this
code path hit and afaict, it happens when multiple frames are loaded at the same
time (the script that hit this code was from an iframe). (Added a comment to
clarify it.)

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:237: // the running task can block
and wait for data from the network.
On 2014/08/17 16:05:26, haraken wrote:
> 
> Just to confirm: Is this the reason it's not easy to support
not-parser-blocking
> scripts?

There are several modifications needed when we want to parse non-parser-blocking
scripts:
- The biggest, as you said, is here; we need to implement some kind of a thread
pool and find the optimal number of threads.
- The script waiting / async execution code path in HTMLScriptRunner needs to be
modified too.

All in all, I think streaming parsing blocking scripts is a reasonable start.
- This CL is big as is, I wouldn't want to add more features to the first
version.
- Parsing blocking scripts are the most critical scripts to get parsed fast; for
other scripts the trade-off will be less (as there might be some reasonable work
to be done while waiting for the script).

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:249: ScriptState* scriptState =
ScriptState::forMainWorld(frame);
On 2014/08/17 16:05:26, haraken wrote:
> 
> Just to confirm: Script streaming is used only in the main world, right?

Only for parser blocking scripts of HTMLScriptRunner; that's the main world,
right?

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:264:
V8ScriptRunner::CacheDecider(0, resource, v8CacheOptions,
&streamer->m_cachedDataDataType, &compileOption, &ignore);
On 2014/08/17 16:05:25, haraken wrote:
> 
> I don't fully understand why you can decide whether you should use V8
> compilation cache or not at this point. Without loading the full source code,
it
> wouldn't be possible to make the decision.
> 
> This is one of my main concerns about this CL.

(As discussed offline), this code path only decides what kind of cached data to
*produce*. Added a comment to clarify that.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:291: // completed.
On 2014/08/17 16:05:26, haraken wrote:
> 
> Can we create a helper method like notifyFinishedToHTMLScriptRunner()?
> 
> void notifyFinishedToHTMLScriptRunner() {
>   if (m_v8Done && m_loadingDone)
>     m_runner->notifyFinished(m_script->resource());
> }

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:291: // completed.
On 2014/08/17 16:05:26, haraken wrote:
> 
> Help me understand: In what scenario is it possible that m_v8Done becomes true
> before m_loadingDone becomes true? Does it happen only when the streaming
> processing is cancelled explicitly?

The V8 side can finish before loading e.g., when V8 detects a parse error.
(Added a comment to clarify that.)

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:301: if (refCount() == 1) {
On 2014/08/17 16:05:25, haraken wrote:
> 
> It's fragile to rely on the reference counter to judge if the PendingScript is
> gone. Can you avoid using refCount() and instead use a more explicit way to
> judge the fact?

Done (I ended up needing a more explicit way anyway, when I fixed reloading
scripts).

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:315: // completed.
On 2014/08/17 16:05:26, haraken wrote:
> 
> Ditto. Use notifyFinishedToHTMLScriptRunner().

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:335: , m_source(m_stream) // Source
takes ownership of m_stream.
On 2014/08/17 16:05:26, haraken wrote:
> 
> BTW, "StreamedSource" and "SourceStream" are confusing. Can we rename to
> something better?
> 
> StreamedSource => SourceStreamController ?

By StreamedSource you mean v8::ScriptCompiler::StreamedSource, right?

StreamedSource is not a SourceStreamController... it doesn't control anything.
It's just a "script source code" object which happens to be streamed. Normally,
the thing given to v8::ScriptCompiler is v8::ScriptCompiler::Source (the source
code of a script) and then we compile that and get a v8::Script back.

So that's why I called v8::ScriptCompiler::StreamedSource like that, since
that's also a Source, it's just streamed. It contains other information
typically associated with script source code too, such as the cached data.

IOW, I'm willing to rename things in general, just that I don't think
SourceStreamController is the right name. :)

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:335: , m_source(m_stream) // Source
takes ownership of m_stream.
On 2014/08/17 16:05:26, haraken wrote:
> 
> Given that StreamedSource has an ownership of the SourceStream, isn't there
any
> way to get the SourceStream from the StreamedSource?
> 
> Then we can remove the m_stream member from V8ScriptStreamer and write the
above
> code like this.
> 
>   m_source(adoptPtr(new SourceStream(script->resource())))

I could add an accessor for getting the ExternalSourceStream* (abstract class)
from StreamedSource, but the caller would need to reinterpret_cast that to the
concrete SourceStream. Would you prefer that?

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:338: { }
On 2014/08/17 16:05:26, haraken wrote:
> 
> Nit: Write { and } in separate lines.

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptStreamer.h (right):

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.h:2: * Copyright (C) 2014 Google Inc.
All rights reserved.
On 2014/08/17 16:05:27, haraken wrote:
> 
> Nit: Use the 3-line copyright.
> 
> // Copyright 2014 The Chromium Authors. All rights reserved.
> // Use of this source code is governed by a BSD-style license that can be
> // found in the LICENSE file.

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.h:47: class V8ScriptStreamer : public
RefCounted<V8ScriptStreamer> {
On 2014/08/17 16:05:27, haraken wrote:
> 
> V8ScriptStreamer => ScriptStreamer
> 
> We're planning to drop a "V8" prefix from V8-binding classes, so let's not add
> the prefix to newly introduced classes.

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.h:47: class V8ScriptStreamer : public
RefCounted<V8ScriptStreamer> {
On 2014/08/17 16:05:27, haraken wrote:
> 
> (We might want to move V8ScriptStreamer to oilpan's heap, but it's OK for now
to
> use a RefPtr. As far as I see, it looks like that there are a couple of
> non-trivial issues in moving V8ScriptStreamer to oilpan's heap.)

Acknowledged.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.h:49: static bool
startStreaming(PendingScript*, HTMLScriptRunner*);
On 2014/08/17 16:05:27, haraken wrote:
> 
> Add a comment about what startStreaming() does.

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.h:70: V8ScriptStreamer(PendingScript*,
HTMLScriptRunner*);
On 2014/08/17 16:05:27, haraken wrote:
> 
> Why do both V8ScriptStreamer's constructor and startStreaming() need to take
the
> same parameters? Can't the startStreaming() use the PendingScript and
> HTMLScriptRunner that are passed to V8ScriptStreamer's constructor?

startStreaming is a static function which constructs a V8ScriptStreamer
internally and attaches it to the PendingScript. So it's not supposed to be used
like "first create a V8ScriptStreamer and then call start on it".

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptStreamerThread.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.cpp:2: * Copyright (C) 2014
Google Inc. All rights reserved.
On 2014/08/17 16:05:27, haraken wrote:
> 
> Nit: Use the 3-line copyright.

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.cpp:41: ASSERT(!s_sharedThread);
On 2014/08/17 16:05:27, haraken wrote:
> 
> Add ASSERT(isMainThread()).

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.cpp:47: ASSERT(s_sharedThread);
On 2014/08/17 16:05:27, haraken wrote:
> 
> Add ASSERT(isMainThread()).

That doesn't seem to be true. :/ It's not true in HTMLParserThread::shutdown()
either.

(I suspect it's a red herring, something like we cannot retrieve sane thread IDs
at this point of the shutdown. But I don't know for sure.)

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.cpp:51: &&
s_sharedThread->isRunningTask()) {
On 2014/08/17 16:05:27, haraken wrote:
> 
> Why is the s_sharedThread->isRunningTask() check needed?
> 
> Probably can we completely get rid of the m_runningTask flag?

Can't it be that Blink is ramping down but the V8 side is still processing data
(that is, the thread is running the task)? I thought that that's possible; thus
this code. Am I misinformed?

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.cpp:72: m_runningTask = true;
On 2014/08/17 16:05:27, haraken wrote:
> 
> Add ASSERT(isMainThread()).

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptStreamerThread.h (right):

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.h:2: * Copyright (C) 2014 Google
Inc. All rights reserved.
On 2014/08/17 16:05:27, haraken wrote:
> 
> Nit: Use the 3-line copyright.

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.h:40: class
V8ScriptStreamerThread {
On 2014/08/17 16:05:27, haraken wrote:
> 
> V8ScriptStreamerThread => ScriptStreamerThread
> 
> We don't want to have a prefix "V8" for newly introduced classes.

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.h:75: // at a time. TODO(marja):
Use a thread pool and stream multiple scripts.
On 2014/08/17 16:05:27, haraken wrote:
> 
> Nit: TODO(marja) => FIXME.

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/core/Init.cpp
File Source/core/Init.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/core/Init.cpp#ne...
Source/core/Init.cpp:132: V8ScriptStreamerThread::shutdown();
On 2014/08/17 16:05:28, haraken wrote:
> 
> We want to shutdown threads in the reverse order of init() calls. So let's
call
> V8ScriptStreamerThread::shutdown() and then HTMLParserThread::shutdown().

Done. I guess it'd only make a difference if the threads post tasks to each
other, and even then, that can only go one way... it's not like both can post
tasks to each other. Or is there some more realistic use case where it would
matter?

(I was also wondering if we need to shut down the thread before destroying the
Isolate in shutdown() in WebKit.cpp, but I guess not, since the background
parser thread doesn't access the isolate.)

https://codereview.chromium.org/368283002/diff/620001/Source/core/dom/Pending...
File Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/core/dom/Pending...
Source/core/dom/PendingScript.cpp:38: m_streamer->cancel();
On 2014/08/17 16:05:28, haraken wrote:
> 
> Doing something non-trivial in a destructor is likely to cause an issue in
> oilpan builds (although there would be no issue in this particular case at
this
> point).
> 
> Can you:
> 
> - define PendingScript::dispose() and call m_streamer->cancel() in the
> PendingScript::dispose()
> - call PendingScript::dispose() from HTMLScriptRunner::detach()
> 
> ?

This code turned out to be unnecessary after I refactored watchForLoad and
stopWatchingForLoad (see a later comment about refactoring them). So now
HTMLScriptRunner simply calls stopWatchingForLoad and that will call cancel().

https://codereview.chromium.org/368283002/diff/620001/Source/core/dom/Pending...
File Source/core/dom/PendingScript.h (right):

https://codereview.chromium.org/368283002/diff/620001/Source/core/dom/Pending...
Source/core/dom/PendingScript.h:108: m_streamer =
RefPtr<V8ScriptStreamer>(streamer);
On 2014/08/17 16:05:28, haraken wrote:
> 
> RefPtr<V8ScriptStreamer> wouldn't be needed.

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/core/dom/Pending...
Source/core/dom/PendingScript.h:111: PassRefPtr<V8ScriptStreamer>
releaseStreamer() { return m_streamer.release(); }
On 2014/08/17 16:05:28, haraken wrote:
> 
> Is it possible that releaseStreamer() is called before
> streamer->notifyFinished() is called? If so, that seems problematic because no
> one will call streamer->notifyFinished() after the PendingScript releases the
> streamer. If no, can we add an ASSERT about it?
> 
> The point is that we want to make sure that notifyFinished is certainly
called.
> Otherwise, it will cause dead lock.

I removed releaseStreamer, since the ownership dance was more complicated than
needed. So PendingScript continues to hold a reference.

However, when we actually release the streamer in setScriptResource,  it must be
that it's not streaming... so I added an assert there.

https://codereview.chromium.org/368283002/diff/620001/Source/core/fetch/Scrip...
File Source/core/fetch/ScriptResource.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/core/fetch/Scrip...
Source/core/fetch/ScriptResource.cpp:52: void
ScriptResource::didAddClient(ResourceClient* c)
On 2014/08/17 16:05:28, haraken wrote:
> 
> Nit: c => client

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/core/fetch/Scrip...
Source/core/fetch/ScriptResource.cpp:61:
ResourceClientWalker<ScriptResourceClient> w(m_clients);
On 2014/08/17 16:05:28, haraken wrote:
> 
> Nit: w => walker

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/core/fetch/Scrip...
Source/core/fetch/ScriptResource.cpp:62: while (ScriptResourceClient* c =
w.next()) {
On 2014/08/17 16:05:28, haraken wrote:
> 
> Nit: c => client

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/core/html/parser...
File Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:78: // streaming).
On 2014/08/17 16:05:28, haraken wrote:
> 
> It's a bit weird that the lifetime of not-streamed loading is taken care of by
> the HTMLScriptRunner whereas the lifetime of streamed loading is taken care of
> by PendingScripts. Can we refactor the code so that all loadings are taken
care
> of by PendingScripts?
> 
> (I guess you'll anyway need to make the change when you support
> not-parser-blocking scripts.)

I don't think it's accurate to say "lifetime of not-streamed loading is taken
care of by the HTMLScriptRunner whereas the lifetime of streamed loading is
taken care of by PendingScripts".

However, HTMLScriptRunner is *watching* non-streamed loads of parsing blocking
scripts, but not watching streamed loads, and not watching the load of
non-parsing-blocking scripts. (Thus functions like
HTMLScriptRunner::watchForLoad and stopWatchingForLoad.) So it's not about
managing the loading itself, but about watching it.

I can unify this though: ScriptStreamer can imitate a Resource so that the
HTMLScriptRunner doesn't know whether it's watching a Resource load or a
ScriptStreamer load. It'll just delegate the "watch" and "stopWatching" to
PendingScript, which will know what to do. I implemented that version and it
turned out pretty nice. Wdyt? I can also do this as a separate CL before the
actual streaming.

https://codereview.chromium.org/368283002/diff/620001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:118: ASSERT(!script.isStreaming());
On 2014/08/17 16:05:28, haraken wrote:
> 
> Help me understand: Why is it guaranteed that script.isStreaming() is false at
> this point?

Because this function is only called when we want to compile the script, so,
streaming must have finished before that. (This can only be called after
HTMLScriptRunner has gotten the "finished" notification.)

(We cannot extract the source code from a resource which is still loading.)

https://codereview.chromium.org/368283002/diff/620001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:203: // when the streaming has
completed too.
On 2014/08/17 16:05:28, haraken wrote:
> 
> This comment is a bit misleading. Slightly better:
> 
> // For a parser-blocking script, this function should be called only when the
> streaming is complete.

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:354: // TODO(marja): in addition to
parser blocking scripts, stream also
On 2014/08/17 16:05:28, haraken wrote:
> 
> Nit: TODO(marja) => FIXME

Done.

https://codereview.chromium.org/368283002/diff/620001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:355: // scripts which are not
parser blocking.
On 2014/08/17 16:05:28, haraken wrote:
> 
> I wonder if there is any particular reason you cannot enable the streaming for
> not-parser-blocking scripts. I'm fine with leaving this as a FIXME for a
> separate CL, but I'm curious if there is any particular reason you cannot do
it
> easily.

(See the other comment.)

https://codereview.chromium.org/368283002/diff/620001/Source/core/xml/parser/...
File Source/core/xml/parser/XMLDocumentParser.h (right):

https://codereview.chromium.org/368283002/diff/620001/Source/core/xml/parser/...
Source/core/xml/parser/XMLDocumentParser.h:68: class XMLDocumentParser FINAL :
public ScriptableDocumentParser, public ScriptResourceClient {
On 2014/08/17 16:05:28, haraken wrote:
> 
> Why does XMLDocumentParser need to be a ScriptResourceClient?

Because it's added as a client to ScriptResources. Previously ScriptResources
just had normal ResourceClients, but now we have ScriptResourceClient
separately, and we want to assert that we get the right type of clients (because
the raw pointers are cast to the correct ResourceClient subclass).

[haraken, 2014-08-20 15:00:19]

Thanks for the hard work! Now the overall approach looks good to me. (The only
question I have is how to shutdown the streaming thread; see the comment below.)

I think that the next step would be to collect performance numbers on some
benchmarks and real-world websites. If the performance numbers are good, I'm
happy to take another close look at the CL.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptSourceCode.h (right):

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptSourceCode.h:94: RefPtr<V8ScriptStreamer>
m_streamer;
On 2014/08/20 11:45:54, marja wrote:
> On 2014/08/17 16:05:25, haraken wrote:
> > 
> > In my understanding, V8ScriptStreamer is owned by either PendingScript (if
the
> > script is still pending) or ScriptSourceCode (if the script is going to be
> > compiled). If so, can we just use an OwnPtr for V8ScriptStreamer? Do we need
> to
> > use RefPtrs?
> 
> I don't think we can get away without RefPtrs; the main reason is the
situation
> when we're streaming.
> 
> So we have a PendingScript and then we're also streaming the script to V8.
> PendingScript can go away any time, but as the V8 side is still accessing the
> V8Streamer (or rather, the SourceStream it holds), it must be kept alive. So
> PendingScript cannot own it. Actually, nothing on the Blink side can own it at
> that point, since basically everything on the Blink side can ramp down and we
> still need to give the V8 background thread some time to realize that.
> 
> Then again, when the V8 side operations have stopped, the PendingScript can
own
> the V8Streamer again (if the PendingScript goes away, it can be destructed).
> 
> So it's logical that at this is handled with references; both PendingScript
and
> the V8 side hold a reference to V8ScriptStreamer. (The V8 side reference is
now
> tracked with explicit ref / deref.)
> 
> However, I simplified this so that we don't have
> PendingScript::releaseStreamer(), so that PendingScript will also hold a
> reference until it's deleted (or set to track a different resource) - it's
> simpler that way.

Makes sense, thanks for the clarification!

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptStreamer.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:237: // the running task can block
and wait for data from the network.
On 2014/08/20 11:45:55, marja wrote:
> On 2014/08/17 16:05:26, haraken wrote:
> > 
> > Just to confirm: Is this the reason it's not easy to support
> not-parser-blocking
> > scripts?
> 
> There are several modifications needed when we want to parse
non-parser-blocking
> scripts:
> - The biggest, as you said, is here; we need to implement some kind of a
thread
> pool and find the optimal number of threads.
> - The script waiting / async execution code path in HTMLScriptRunner needs to
be
> modified too.
> 
> All in all, I think streaming parsing blocking scripts is a reasonable start.
> - This CL is big as is, I wouldn't want to add more features to the first
> version.
> - Parsing blocking scripts are the most critical scripts to get parsed fast;
for
> other scripts the trade-off will be less (as there might be some reasonable
work
> to be done while waiting for the script).

Understood.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:249: ScriptState* scriptState =
ScriptState::forMainWorld(frame);
On 2014/08/20 11:45:55, marja wrote:
> On 2014/08/17 16:05:26, haraken wrote:
> > 
> > Just to confirm: Script streaming is used only in the main world, right?
> 
> Only for parser blocking scripts of HTMLScriptRunner; that's the main world,
> right?

Yes, I believe so. I also believe that non-parser-blocking scripts are
guaranteed to be executed in the main world as well.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:335: , m_source(m_stream) // Source
takes ownership of m_stream.
On 2014/08/20 11:45:55, marja wrote:
> On 2014/08/17 16:05:26, haraken wrote:
> > 
> > BTW, "StreamedSource" and "SourceStream" are confusing. Can we rename to
> > something better?
> > 
> > StreamedSource => SourceStreamController ?
> 
> By StreamedSource you mean v8::ScriptCompiler::StreamedSource, right?
> 
> StreamedSource is not a SourceStreamController... it doesn't control anything.
> It's just a "script source code" object which happens to be streamed.
Normally,
> the thing given to v8::ScriptCompiler is v8::ScriptCompiler::Source (the
source
> code of a script) and then we compile that and get a v8::Script back.
> 
> So that's why I called v8::ScriptCompiler::StreamedSource like that, since
> that's also a Source, it's just streamed. It contains other information
> typically associated with script source code too, such as the cached data.
> 
> IOW, I'm willing to rename things in general, just that I don't think
> SourceStreamController is the right name. :)

Got it; StreamedSource makes sense.

However, it's still confusing that we also have "SourceStream" inherited from
v8::ExternalSourceStream. Can you come up with a better name than SourceStream?

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:335: , m_source(m_stream) // Source
takes ownership of m_stream.
On 2014/08/20 11:45:55, marja wrote:
> On 2014/08/17 16:05:26, haraken wrote:
> > 
> > Given that StreamedSource has an ownership of the SourceStream, isn't there
> any
> > way to get the SourceStream from the StreamedSource?
> > 
> > Then we can remove the m_stream member from V8ScriptStreamer and write the
> above
> > code like this.
> > 
> >   m_source(adoptPtr(new SourceStream(script->resource())))
> 
> I could add an accessor for getting the ExternalSourceStream* (abstract class)
> from StreamedSource, but the caller would need to reinterpret_cast that to the
> concrete SourceStream. Would you prefer that?

OK, then let's keep your current code as is.

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptStreamerThread.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.cpp:47: ASSERT(s_sharedThread);
On 2014/08/20 11:45:56, marja wrote:
> On 2014/08/17 16:05:27, haraken wrote:
> > 
> > Add ASSERT(isMainThread()).
> 
> That doesn't seem to be true. :/ It's not true in HTMLParserThread::shutdown()
> either.
> 
> (I suspect it's a red herring, something like we cannot retrieve sane thread
IDs
> at this point of the shutdown. But I don't know for sure.)

OK :)

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.cpp:51: &&
s_sharedThread->isRunningTask()) {
On 2014/08/20 11:45:56, marja wrote:
> On 2014/08/17 16:05:27, haraken wrote:
> > 
> > Why is the s_sharedThread->isRunningTask() check needed?
> > 
> > Probably can we completely get rid of the m_runningTask flag?
> 
> Can't it be that Blink is ramping down but the V8 side is still processing
data
> (that is, the thread is running the task)? I thought that that's possible;
thus
> this code. Am I misinformed?

I agree that it's possible that the streaming thread is running a task when the
main thread wants to shut down the streaming thread. However, in that case, is
it safe to forcibly delete the streaming thread without waiting for the task
completion? (I don't think it's safe.)

We should probably send a cancel notification to the streaming thread, wait for
the streaming thread to process the cancel task, and then gracefully shut down
the streaming thread. (I don't know how much it will block the main thread. Even
if it's a shutdown sequence, we're not allowed to block the main thread for a
long time.)

https://codereview.chromium.org/368283002/diff/620001/Source/core/Init.cpp
File Source/core/Init.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/core/Init.cpp#ne...
Source/core/Init.cpp:132: V8ScriptStreamerThread::shutdown();
On 2014/08/20 11:45:56, marja wrote:
> On 2014/08/17 16:05:28, haraken wrote:
> > 
> > We want to shutdown threads in the reverse order of init() calls. So let's
> call
> > V8ScriptStreamerThread::shutdown() and then HTMLParserThread::shutdown().
> 
> Done. I guess it'd only make a difference if the threads post tasks to each
> other, and even then, that can only go one way... it's not like both can post
> tasks to each other. Or is there some more realistic use case where it would
> matter?
> 
> (I was also wondering if we need to shut down the thread before destroying the
> Isolate in shutdown() in WebKit.cpp, but I guess not, since the background
> parser thread doesn't access the isolate.)

I don't know any specific cases where the order matters, but the current
shutdown logic is carefully written in the exactly reverse order of init()
calls. So let's just follow the rule here.

https://codereview.chromium.org/368283002/diff/620001/Source/core/html/parser...
File Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:78: // streaming).
On 2014/08/20 11:45:57, marja wrote:
> On 2014/08/17 16:05:28, haraken wrote:
> > 
> > It's a bit weird that the lifetime of not-streamed loading is taken care of
by
> > the HTMLScriptRunner whereas the lifetime of streamed loading is taken care
of
> > by PendingScripts. Can we refactor the code so that all loadings are taken
> care
> > of by PendingScripts?
> > 
> > (I guess you'll anyway need to make the change when you support
> > not-parser-blocking scripts.)
> 
> I don't think it's accurate to say "lifetime of not-streamed loading is taken
> care of by the HTMLScriptRunner whereas the lifetime of streamed loading is
> taken care of by PendingScripts".
> 
> However, HTMLScriptRunner is *watching* non-streamed loads of parsing blocking
> scripts, but not watching streamed loads, and not watching the load of
> non-parsing-blocking scripts. (Thus functions like
> HTMLScriptRunner::watchForLoad and stopWatchingForLoad.) So it's not about
> managing the loading itself, but about watching it.
> 
> I can unify this though: ScriptStreamer can imitate a Resource so that the
> HTMLScriptRunner doesn't know whether it's watching a Resource load or a
> ScriptStreamer load. It'll just delegate the "watch" and "stopWatching" to
> PendingScript, which will know what to do. I implemented that version and it
> turned out pretty nice. Wdyt? I can also do this as a separate CL before the
> actual streaming.

The new version looks nicer to me!

[marja, 2014-09-09 15:30:43]

Patchset #46 (id:900001) has been deleted

[marja, 2014-09-09 17:00:26]

haraken: the V8 side is reviewed now; ptal of this CL

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptStreamer.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamer.cpp:335: , m_source(m_stream) // Source
takes ownership of m_stream.
I tried to think of the naming a bit, but didn't figure out a better name yet. I
guess the V8 side should change too, so, instead of ExternalSourceStream it
would be something else..

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptStreamerThread.cpp (right):

https://codereview.chromium.org/368283002/diff/620001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptStreamerThread.cpp:51: &&
s_sharedThread->isRunningTask()) {
On 2014/08/20 15:00:18, haraken wrote:
> On 2014/08/20 11:45:56, marja wrote:
> > On 2014/08/17 16:05:27, haraken wrote:
> > > 
> > > Why is the s_sharedThread->isRunningTask() check needed?
> > > 
> > > Probably can we completely get rid of the m_runningTask flag?
> > 
> > Can't it be that Blink is ramping down but the V8 side is still processing
> data
> > (that is, the thread is running the task)? I thought that that's possible;
> thus
> > this code. Am I misinformed?
> 
> I agree that it's possible that the streaming thread is running a task when
the
> main thread wants to shut down the streaming thread. However, in that case, is
> it safe to forcibly delete the streaming thread without waiting for the task
> completion? (I don't think it's safe.)

Yes, I don't think it's safe either, and that's why I was confused since I
thought you just suggested that above :) Ahh, but now I think I understand what
you mean: that we should still have this code to carefully ramp down the
streaming thread, but we shouldn't check isRunningTask() in the above if? Can
there be a race that isRunningTask() is false and we decide to not run this
code, but a task gets scheduled in between - I don't think so, since the main
thread is scheduling all the tasks. However, it's always safe to run this ramp
down code (maybe in the future some non-main threads might schedule streaming
tasks?), so I can remove the isRunningTask() check from the if.

> We should probably send a cancel notification to the streaming thread, wait
for
> the streaming thread to process the cancel task, and then gracefully shut down
> the streaming thread. (I don't know how much it will block the main thread.
Even
> if it's a shutdown sequence, we're not allowed to block the main thread for a
> long time.)

This is essentially what this code is doing. When the script is no longer
needed, the call chain is: HTMLScriptRunner::detach ->
PendingScript::stopWatchingForLoad -> ScriptStreamer::cancel, so, we have
already cancelled, but the V8 side will realize the cancellation only when it
requests the data the next time. So here we make sure we give it time to realize
it and ramp down its own operations before we shut down the thread.

[marja, 2014-09-09 17:05:02]

In addition, a note about testing:

I'd sincerely like to test ScriptStreamer, but I don't see how to :/ Interesting
tests would be e.g., "make sure scripts get ran fine if the loading finished way
before V8" and "make sure the Blink side handles it gracefully if the V8 side
finished before the loading (this means a parse error)". But that would mean I
need some mechanism to control how long the V8 side task takes, and... well..
that's essentially mocking V8. I don't see a clean way to do that. Are there any
tests testing similar kind of race conditions in other parts of Blink ("what if
we navigate away before resource loading finishes" or such)? Do we have any
testing infra to support those cases?

[marja, 2014-09-09 17:12:14]

A clarifying note: Technically I wouldn't need to mock V8 (since Blink has the
task), but I'd need to do things like making Resource (or its SharedBuffer)
return the amount of data I want it to return, and I'd need to mock / add test
code into the Blink side ScriptStreamingTask.

[haraken, 2014-09-10 04:37:42]

On 2014/09/09 17:12:14, marja wrote:
> A clarifying note: Technically I wouldn't need to mock V8 (since Blink has the
> task), but I'd need to do things like making Resource (or its SharedBuffer)
> return the amount of data I want it to return, and I'd need to mock / add test
> code into the Blink side ScriptStreamingTask.

I don't think you need to test all racing cases, but we need a test for this by
adding some mocks.

I'm not sure if this is a good idea, but can we test this by adding a huge
JavaScript file to http/tests/ of layout tests?

Started looking the CL in details.

[haraken, 2014-09-10 04:59:18]

On 2014/09/10 04:37:42, haraken wrote:
> On 2014/09/09 17:12:14, marja wrote:
> > A clarifying note: Technically I wouldn't need to mock V8 (since Blink has
the
> > task), but I'd need to do things like making Resource (or its SharedBuffer)
> > return the amount of data I want it to return, and I'd need to mock / add
test
> > code into the Blink side ScriptStreamingTask.
> 
> I don't think you need to test all racing cases, but we need a test for this
by
> adding some mocks.
> 
> I'm not sure if this is a good idea, but can we test this by adding a huge
> JavaScript file to http/tests/ of layout tests?

^^^ Hmm, ignore this comment. Testing this feature in layout tests will just
produce a very flaky result. Unit testing would be the best.

[haraken, 2014-09-10 05:57:52]

Mostly looks good (except that this CL needs a good test).

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptStreamer.cpp (right):

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.cpp:26: class SourceStream : public
v8::ScriptCompiler::ExternalSourceStream {

I'd like to have V8 reviewers to take a look at this class.

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.cpp:41: // Called by V8 on a background
thread. Should block until we can return

Can we add an ASSERT about the thread correctness? ASSERT(!isMainThread()) or
something better.

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.cpp:63: // Called on the main thread.

ASSERT(isMainThread()). The same comment for other parts of this CL.

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.cpp:225: if
(!resource->url().protocolIsInHTTPFamily()) {

Just help me understand: What is this check for?

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.cpp:247: // purposes.

If the script has a comment written in Japanese, it won't run in the
streaming... ok :)

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.cpp:270: if (scriptStreamingTask) {

When can scriptStreamingTask be 0?

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.cpp:310: m_resource = 0;

As mentioned in the header file, it would be better if we could remove the
m_resource member from ScriptStreamer.

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptStreamer.h (right):

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.h:42: // more. After calling cancel, the
upper layer (PendingScript) can drop its

This comment is a bit confusing, though I know what you mean. What does the
"upper layer" mean? Is it a PendingScript?

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.h:86: ScriptResource* m_resource;

Can we avoid adding the m_resource member to ScriptStreamer? In cases where a
ScriptStreamer wants to use a ScriptResource, you can pass the ScriptResource
explicitly to the ScriptStreamer's methods.

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.h:92: bool m_v8Done; // Whether the V8
side processing is done.

m_v8Done => m_parsingFinished ? (for consistency with m_loadingFinished)

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.h:99: unsigned m_cachedDataDataType;

m_cachedDataDataType => m_cachedDataType ?

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptStreamerThread.cpp (right):

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamerThread.cpp:37:
s_sharedThread->postTask(new
Task(WTF::bind(&ScriptStreamerThread::markAsCompleted, s_sharedThread,
&taskSynchronizer)));

It's not nice to block the shutdown sequence by waiting for V8 to complete the
remaining parsing task, but I'm OK with this because the situation will rarely
happen.

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamerThread.cpp:52: m_runningTask = true;

Shall we add ASSERT(!m_runningTask) before this line?

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptStreamerThread.h (right):

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamerThread.h:34: shared()->m_runningTask =
false;

Shall we add ASSERT(shared()->m_runningTask) before this line?

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamerThread.h:66: ScriptStreamer* m_streamer;

Doesn't this need to be a RefPtr?

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:135: cacheTag = tagForParserCache();

Can you use this tagForParserCache() instead of introducing another
tagForParserCache()?

https://codereview.chromium.org/368283002/diff/950001/Source/core/dom/Pending...
File Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/368283002/diff/950001/Source/core/dom/Pending...
Source/core/dom/PendingScript.cpp:62: } else if (resource()) {

How is it possible that resource() returns 0 here?

I guess this branch should be:

  else {
    ASSERT(resource());
  }

https://codereview.chromium.org/368283002/diff/950001/Source/core/dom/Pending...
Source/core/dom/PendingScript.cpp:82: ASSERT(!m_streamer ||
!m_streamer->streamingInProgress());

ASSERT(!isStreaming());

https://codereview.chromium.org/368283002/diff/950001/Source/core/dom/Pending...
Source/core/dom/PendingScript.cpp:82: ASSERT(!m_streamer ||
!m_streamer->streamingInProgress());

Add ASSERT(!m_watchingForLoad);

https://codereview.chromium.org/368283002/diff/950001/Source/core/dom/Pending...
Source/core/dom/PendingScript.cpp:110: if (m_streamer &&
!m_streamer->streamingSuppressed()) {

Help me understand: Why do we need this branch? Can we just always return
ScriptSourceCode(resource()) ?

I think I just don't understand how the suppressing works.

https://codereview.chromium.org/368283002/diff/950001/Source/core/html/parser...
File Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/368283002/diff/950001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:110: if (script.resource() &&
!script.resource()->isLoaded())

Not related to this CL, is it possible that script.resource() is 0 here?

If script.resource() is 0, isPendingScriptReady returns true, which sounds very
strange.

https://codereview.chromium.org/368283002/diff/950001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:266:
blink::Platform::current()->histogramEnumeration("WebCore.Scripts.ParsingBlocking.StartedStreaming",
startedStreaming ? 1 : 0, 2);

You can add this in a separate CL.

[marja, 2014-09-11 09:15:38]

Thanks for having a look again. I've now uploaded a patch set, the changes are:

1) added tests

2) code review comments - my intention was to do / comment on all of them

3) fix for reloading pages:

When we're reloading, we do Resource::setResourceToRevalidate(old resource) and
then eventually migrate all clients to watch the old resource
(Resource::switchClientsToRevalidatedResource). So now, ScriptStreamer had tried
to stream from one resource (but failed), and then it got the finished
notification for another resource and was confused. I added code to
ScriptStreamer::startStreaming to not stream if
Resource::setResourceToRevalidate has been called. This also makes the code in
prepareDataOnMainThread simpler. And I added ASSERTs to notifyAppendData and
notifyFinished to make sure that ScriptStreamer thinks it's watching the correct
resource.


jochen@: could you review the following parts:

1) ScriptStreamer.{h,cpp}; haraken@ was asking for somebody from the V8 side to
review them.

2) OWNERs review for Source/web/WebSettingsImpl.{h,cpp} and
public/web/WebSettings.h

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptStreamer.cpp (right):

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.cpp:26: class SourceStream : public
v8::ScriptCompiler::ExternalSourceStream {
On 2014/09/10 05:57:51, haraken wrote:
> 
> I'd like to have V8 reviewers to take a look at this class.

Acknowledged; will ask jochen@.

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.cpp:41: // Called by V8 on a background
thread. Should block until we can return
On 2014/09/10 05:57:51, haraken wrote:
> 
> Can we add an ASSERT about the thread correctness? ASSERT(!isMainThread()) or
> something better.

Done.

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.cpp:63: // Called on the main thread.
On 2014/09/10 05:57:51, haraken wrote:
> 
> ASSERT(isMainThread()). The same comment for other parts of this CL.

Done.

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.cpp:225: if
(!resource->url().protocolIsInHTTPFamily()) {
On 2014/09/10 05:57:51, haraken wrote:
> 
> Just help me understand: What is this check for?

So that we don't stream e.g., file:///stuff.

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.cpp:247: // purposes.
On 2014/09/10 05:57:51, haraken wrote:
> 
> If the script has a comment written in Japanese, it won't run in the
> streaming... ok :)

Unless it's UTF-8 :)

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.cpp:270: if (scriptStreamingTask) {
On 2014/09/10 05:57:51, haraken wrote:
> 
> When can scriptStreamingTask be 0?

When V8 refuses streaming; added a comment about that.

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.cpp:310: m_resource = 0;
On 2014/09/10 05:57:51, haraken wrote:
> 
> As mentioned in the header file, it would be better if we could remove the
> m_resource member from ScriptStreamer.

Here the m_resource == 0 was carrying a special meaning ("streamer detached"),
so I added a boolean to track that.

(However, couldn't get rid of the resource pointer, see the other comment reply
for clarification. :/ )

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptStreamer.h (right):

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.h:42: // more. After calling cancel, the
upper layer (PendingScript) can drop its
On 2014/09/10 05:57:51, haraken wrote:
> 
> This comment is a bit confusing, though I know what you mean. What does the
> "upper layer" mean? Is it a PendingScript?

Clarified this:

    // Called when the script is not needed any more (e.g., loading was
    // cancelled). After calling cancel, PendingScript can drop its reference to
    // ScriptStreamer, and ScriptStreamer takes care of eventually deleting
    // itself (after the V8 side has finished too).

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.h:86: ScriptResource* m_resource;
On 2014/09/10 05:57:51, haraken wrote:
> 
> Can we avoid adding the m_resource member to ScriptStreamer? In cases where a
> ScriptStreamer wants to use a ScriptResource, you can pass the ScriptResource
> explicitly to the ScriptStreamer's methods.

We need it in e.g., notifyFinishedToClient which might get called when the V8
side finishes, so at that point, nobody can pass the Resource to ScriptStreamer
(unless ScriptStreamerTask or something like that holds a pointer to it, but I
don't think it's any better).

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.h:92: bool m_v8Done; // Whether the V8
side processing is done.
On 2014/09/10 05:57:51, haraken wrote:
> 
> m_v8Done => m_parsingFinished ? (for consistency with m_loadingFinished)

Done.

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamer.h:99: unsigned m_cachedDataDataType;
On 2014/09/10 05:57:51, haraken wrote:
> 
> m_cachedDataDataType => m_cachedDataType ?

Done.

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptStreamerThread.cpp (right):

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamerThread.cpp:37:
s_sharedThread->postTask(new
Task(WTF::bind(&ScriptStreamerThread::markAsCompleted, s_sharedThread,
&taskSynchronizer)));
On 2014/09/10 05:57:51, haraken wrote:
> 
> It's not nice to block the shutdown sequence by waiting for V8 to complete the
> remaining parsing task, but I'm OK with this because the situation will rarely
> happen.

It doesn't make the situation worse than now: previously the main thread used to
call Parse and while that is happening, nothing else is.

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamerThread.cpp:52: m_runningTask = true;
On 2014/09/10 05:57:51, haraken wrote:
> 
> Shall we add ASSERT(!m_runningTask) before this line?

Done.

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptStreamerThread.h (right):

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamerThread.h:34: shared()->m_runningTask =
false;
On 2014/09/10 05:57:51, haraken wrote:
> 
> Shall we add ASSERT(shared()->m_runningTask) before this line?

Done.

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptStreamerThread.h:66: ScriptStreamer* m_streamer;
On 2014/09/10 05:57:51, haraken wrote:
> 
> Doesn't this need to be a RefPtr?

No, the code is correct as is, afaics (see the manual ref() & deref() in
ScriptStreamer.cpp for keeping ScriptStreamer alive long enough).

And I cannot use RefPtrs on a background thread anyway. If I put a RefPtr here,
I cannot access it on the background thread: not in run() and not in dtor either
(the dtor is called immediately after running the task, and it's called on a
background thread), so there's no possibility to even schedule something on the
main thread that would then access the RefPtr.

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/368283002/diff/950001/Source/bindings/core/v8...
Source/bindings/core/v8/V8ScriptRunner.cpp:135: cacheTag = tagForParserCache();
On 2014/09/10 05:57:51, haraken wrote:
> 
> Can you use this tagForParserCache() instead of introducing another
> tagForParserCache()?

Hmm? I'm not introducing a new tagForParserCache, I'm just moving it from the
unnamed namespace to V8ScriptRunner, so that ScriptStreamer can access them too.

https://codereview.chromium.org/368283002/diff/950001/Source/core/dom/Pending...
File Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/368283002/diff/950001/Source/core/dom/Pending...
Source/core/dom/PendingScript.cpp:62: } else if (resource()) {
On 2014/09/10 05:57:52, haraken wrote:
> 
> How is it possible that resource() returns 0 here?
> 
> I guess this branch should be:
> 
>   else {
>     ASSERT(resource());
>   }

This code was originally in HTMLScriptRunner like this:

if (m_parserBlockingScript.resource() &&
m_parserBlockingScript.watchingForLoad())


and in that case, it's clear that it can be that resource() is NULL. However, I
think that check was unnecessarily complicated, and 

if (m_parserBlockingScript.watchingForLoad())

would've been enough.

Anyhow, added the ASSERT here as you suggested.

https://codereview.chromium.org/368283002/diff/950001/Source/core/dom/Pending...
Source/core/dom/PendingScript.cpp:82: ASSERT(!m_streamer ||
!m_streamer->streamingInProgress());
On 2014/09/10 05:57:52, haraken wrote:
> 
> ASSERT(!isStreaming());

Done.

https://codereview.chromium.org/368283002/diff/950001/Source/core/dom/Pending...
Source/core/dom/PendingScript.cpp:82: ASSERT(!m_streamer ||
!m_streamer->streamingInProgress());
On 2014/09/10 05:57:52, haraken wrote:
> 
> Add ASSERT(!m_watchingForLoad);

Done.

https://codereview.chromium.org/368283002/diff/950001/Source/core/dom/Pending...
Source/core/dom/PendingScript.cpp:110: if (m_streamer &&
!m_streamer->streamingSuppressed()) {
On 2014/09/10 05:57:51, haraken wrote:
> 
> Help me understand: Why do we need this branch? Can we just always return
> ScriptSourceCode(resource()) ?
> 
> I think I just don't understand how the suppressing works.

We need to somehow take the results of streaming into use when compiling, so
this is why we cannot do ScriptSourceCode(resource()) - that would just discard
everything we have done, and we'd end up parsing and compiling the script like
before. We need ScriptSourceCode reflect the fact that we have parsed the code
already and the only thing left is compilation.

Suppressing means that ScriptStreamer is still watching the resource load, just
not giving data to V8. So in those cases, we get the source code via the normal,
non-streaming code path. And from that point on, we just forget that we ever
streamed, and we parse and compile the code like before.

https://codereview.chromium.org/368283002/diff/950001/Source/core/html/parser...
File Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/368283002/diff/950001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:110: if (script.resource() &&
!script.resource()->isLoaded())
On 2014/09/10 05:57:52, haraken wrote:
> 
> Not related to this CL, is it possible that script.resource() is 0 here?
> 
> If script.resource() is 0, isPendingScriptReady returns true, which sounds
very
> strange.

Hmm, no idea.

https://codereview.chromium.org/368283002/diff/950001/Source/core/html/parser...
Source/core/html/parser/HTMLScriptRunner.cpp:266:
blink::Platform::current()->histogramEnumeration("WebCore.Scripts.ParsingBlocking.StartedStreaming",
startedStreaming ? 1 : 0, 2);
On 2014/09/10 05:57:52, haraken wrote:
> 
> You can add this in a separate CL.

Hmm, which part? Calling startStreaming or the UMA? Why? I'll have a Finch
experiment for this, so even if I call startStreaming here, it'll only be
enabled for the finch population, not everybody. And the UMA is just one line,
so I don't see why I should create a separate CL for it.

[marja, 2014-09-11 09:16:04]

marja@chromium.org changed reviewers:
+ jochen@chromium.org

[marja, 2014-09-11 09:16:06]

and now really adding jochen@, pls see the message above

[jochen (gone - plz use gerrit), 2014-09-11 12:45:09]

my parts lgtm with nits

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptSourceCode.h (right):

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptSourceCode.h:65:
ScriptSourceCode(PassRefPtr<ScriptStreamer> streamer, ScriptResource* cs)
what is cs short for?

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptStreamer.cpp (right):

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptStreamer.cpp:26: public:
wtf_make_noncopyable

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptStreamer.cpp:276: streamer->ref();
add a comment that this is balanced in streamingComplete

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptStreamer.cpp:358: if (m_loadingFinished &&
m_parsingFinished && m_client) {
single-line if bodies shouldn't have { } - here and everywhere

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptStreamer.h (right):

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptStreamer.h:29: class ScriptStreamer : public
RefCounted<ScriptStreamer> {
wtf_make_noncopyable

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptStreamerThread.cpp (right):

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptStreamerThread.cpp:36: TaskSynchronizer
taskSynchronizer;
you don't need the task synchronizer, the delete s_sharedThread below will block
until all tasks are finished

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptStreamerThread.h (right):

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptStreamerThread.h:19: class ScriptStreamerThread {
wtf make noncopyable

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptStreamerThread.h:60: class ScriptStreamingTask :
public WebThread::Task {
noncopyable

[haraken, 2014-09-11 16:12:30]

LGTM! This CL is full of nice comments.

I think it would be better to announce this change to blink-dev@.

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/V8ScriptRunner.cpp:116: if (streamer) {

Can we add:

  ASSERT(!m_streamer->streamingSuppressed());
  ASSERT(!m_streamer->streamingInProgress());

?

https://codereview.chromium.org/368283002/diff/1010001/Source/core/dom/Pendin...
File Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/368283002/diff/1010001/Source/core/dom/Pendin...
Source/core/dom/PendingScript.cpp:62: m_streamer->removeClient(client);

Shall we clear m_streamer ?

https://codereview.chromium.org/368283002/diff/1010001/Source/core/dom/Pendin...
Source/core/dom/PendingScript.cpp:112: if (m_streamer &&
!m_streamer->streamingSuppressed()) {

Just to confirm: Is it possible that the streaming is not suppressed at this
point but is suppressed later when V8ScriptRunner::compileScript tries to
compile the ScriptSourceCode. I think the answer is no. If the answer is yes, it
should be problematic.

https://codereview.chromium.org/368283002/diff/1010001/Source/core/html/parse...
File Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/368283002/diff/1010001/Source/core/html/parse...
Source/core/html/parser/HTMLScriptRunner.cpp:266:
blink::Platform::current()->histogramEnumeration("WebCore.Scripts.ParsingBlocking.StartedStreaming",
startedStreaming ? 1 : 0, 2);

Did you already land a chromium side change?

[marja, 2014-09-15 17:45:27]

Thanks for review! Will try to land this now...

haraken@: I don't remember whether we already talked about this, but re:
announcement to blink-dev, we chatted with jochen@ and concluded the most
sensemaking time to post it would be when we enable the feature for a larger
percentage of users via Finch.

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptSourceCode.h (right):

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptSourceCode.h:65:
ScriptSourceCode(PassRefPtr<ScriptStreamer> streamer, ScriptResource* cs)
On 2014/09/11 12:45:08, jochen wrote:
> what is cs short for?

Seems to be copy paste from above. I changed it to "resource" here and above.

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptStreamer.cpp (right):

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptStreamer.cpp:26: public:
On 2014/09/11 12:45:08, jochen wrote:
> wtf_make_noncopyable

Done.

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptStreamer.cpp:276: streamer->ref();
On 2014/09/11 12:45:08, jochen wrote:
> add a comment that this is balanced in streamingComplete

Done.

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptStreamer.cpp:358: if (m_loadingFinished &&
m_parsingFinished && m_client) {
On 2014/09/11 12:45:08, jochen wrote:
> single-line if bodies shouldn't have { } - here and everywhere

Done; but kept the { } in cases where comments make the body multi-line. (For
clarity.)

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptStreamer.h (right):

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptStreamer.h:29: class ScriptStreamer : public
RefCounted<ScriptStreamer> {
On 2014/09/11 12:45:08, jochen wrote:
> wtf_make_noncopyable

Done.

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptStreamerThread.cpp (right):

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptStreamerThread.cpp:36: TaskSynchronizer
taskSynchronizer;
On 2014/09/11 12:45:08, jochen wrote:
> you don't need the task synchronizer, the delete s_sharedThread below will
block
> until all tasks are finished

Hmm, looks like WebThreadImpl dtor indeed waits.

I poked this code a bit, and realized it wasn't doing what I thought it was
doing (copied from HTMLParserThread...) basically, I shut down chrome while a
script was streaming (I made V8 to sleep to make sure I'm really in the middle
of the streaming), and shutdown() for the thread of interest didn't even ever
get called o.O (That doesn't seem to be a mess-up in my code, just that
CoreInitializer::shutdown() doesn't seem to be ran for the process of interest,
or at least my debug prints there don't show. Maybe the renderer shutdown is
more violent... anyhow, beyond the scope of this CL.)

So anyhow, this code is by no means doing what it should, so I deleted it.

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
File Source/bindings/core/v8/ScriptStreamerThread.h (right):

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptStreamerThread.h:19: class ScriptStreamerThread {
On 2014/09/11 12:45:08, jochen wrote:
> wtf make noncopyable

Done.

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/ScriptStreamerThread.h:60: class ScriptStreamingTask :
public WebThread::Task {
On 2014/09/11 12:45:08, jochen wrote:
> noncopyable

Done.

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
File Source/bindings/core/v8/V8ScriptRunner.cpp (right):

https://codereview.chromium.org/368283002/diff/1010001/Source/bindings/core/v...
Source/bindings/core/v8/V8ScriptRunner.cpp:116: if (streamer) {
On 2014/09/11 16:12:30, haraken wrote:
> 
> Can we add:
> 
>   ASSERT(!m_streamer->streamingSuppressed());
>   ASSERT(!m_streamer->streamingInProgress());
> 
> ?

Done.

https://codereview.chromium.org/368283002/diff/1010001/Source/core/dom/Pendin...
File Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/368283002/diff/1010001/Source/core/dom/Pendin...
Source/core/dom/PendingScript.cpp:62: m_streamer->removeClient(client);
On 2014/09/11 16:12:30, haraken wrote:
> 
> Shall we clear m_streamer ?

Yeah, we cancel() it here anyway, so why not. Done.

https://codereview.chromium.org/368283002/diff/1010001/Source/core/dom/Pendin...
Source/core/dom/PendingScript.cpp:112: if (m_streamer &&
!m_streamer->streamingSuppressed()) {
On 2014/09/11 16:12:30, haraken wrote:
> 
> Just to confirm: Is it possible that the streaming is not suppressed at this
> point but is suppressed later when V8ScriptRunner::compileScript tries to
> compile the ScriptSourceCode. I think the answer is no. If the answer is yes,
it
> should be problematic.

You're correct, the answer is no. The streaming cannot be suppressed once it has
completed. Or if it is, it has no effect and makes no sense for anyone to call
suppress when we have already parsed.

https://codereview.chromium.org/368283002/diff/1010001/Source/core/html/parse...
File Source/core/html/parser/HTMLScriptRunner.cpp (right):

https://codereview.chromium.org/368283002/diff/1010001/Source/core/html/parse...
Source/core/html/parser/HTMLScriptRunner.cpp:266:
blink::Platform::current()->histogramEnumeration("WebCore.Scripts.ParsingBlocking.StartedStreaming",
startedStreaming ? 1 : 0, 2);
On 2014/09/11 16:12:30, haraken wrote:
> 
> Did you already land a chromium side change?

No, but I can land this before it without problems. The Chromium side change
only affects the data display so the data will be collected without problems, I
just need to land the histograms.xml change before I need to see it :)

[marja, 2014-09-15 17:46:20]

The CQ bit was checked by marja@chromium.org

[commit-bot: I haz the power, 2014-09-15 17:47:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/368283002/1070001

[commit-bot: I haz the power, 2014-09-15 17:57:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-15 17:57:56]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/56030)

[marja, 2014-09-15 17:59:34]

The CQ bit was checked by marja@chromium.org

[commit-bot: I haz the power, 2014-09-15 17:59:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/368283002/1090001

[commit-bot: I haz the power, 2014-09-15 18:18:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-15 18:18:45]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)

[haraken, 2014-09-15 23:25:14]

> haraken@: I don't remember whether we already talked about this, but re:
> announcement to blink-dev, we chatted with jochen@ and concluded the most
> sensemaking time to post it would be when we enable the feature for a larger
> percentage of users via Finch.

I'm fine with the decision.

[marja, 2014-09-16 07:15:05]

The CQ bit was checked by marja@chromium.org

[commit-bot: I haz the power, 2014-09-16 07:15:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/368283002/1110001

[commit-bot: I haz the power, 2014-09-16 07:37:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-16 07:37:56]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[marja, 2014-09-16 08:14:09]

The CQ bit was checked by marja@chromium.org

[commit-bot: I haz the power, 2014-09-16 08:15:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/368283002/1150001

[commit-bot: I haz the power, 2014-09-16 09:26:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-16 09:26:15]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[marja, 2014-09-16 10:04:22]

The CQ bit was checked by marja@chromium.org

[commit-bot: I haz the power, 2014-09-16 10:04:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/368283002/1170001

[commit-bot: I haz the power, 2014-09-16 11:05:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-16 11:05:26]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[marja, 2014-09-16 11:20:38]

(FYI)

https://codereview.chromium.org/368283002/diff/950001/Source/core/dom/Pending...
File Source/core/dom/PendingScript.cpp (right):

https://codereview.chromium.org/368283002/diff/950001/Source/core/dom/Pending...
Source/core/dom/PendingScript.cpp:82: ASSERT(!m_streamer ||
!m_streamer->streamingInProgress());
On 2014/09/10 05:57:52, haraken wrote:
> 
> Add ASSERT(!m_watchingForLoad);

Actually, this is not true in the current code (independent of script
streaming).

The code path where this is not true:

#3 blink::PendingScript::setScriptResource()
#4 blink::PendingScript::PendingScript()
#5 WTF::Deque<>::takeFirst()
#6 blink::HTMLScriptRunner::executeScriptsWaitingForParsing()
#7 blink::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd()
#8 blink::HTMLDocumentParser::notifyScriptLoaded()

... in general, it's not guaranteed that HTMLScriptRunner calls
stopWatchingForLoad when it has already been notified of the load having
finished.

[marja, 2014-09-16 11:20:52]

The CQ bit was checked by marja@chromium.org

[commit-bot: I haz the power, 2014-09-16 11:21:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/368283002/1190001

[commit-bot: I haz the power, 2014-09-16 12:29:41]

Committed patchset #60 (id:1190001) as 182048