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

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

This makes the main thread block and wait for the parser thread when all the
script data has arrived. The goal is to ensure that the script (which can now be
compiled) will get the main thread's attention as soon as possible.

This feature is temporary: The usefulness of blocking will be evaluated with
Finch, and this option will be removed (blocking will be always enabled or never
enabled, based on which option turns out to be better).

R=haraken@chromium.org
BUG=

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

[marja, 2014-10-14 09:22:10]

haraken, ptal.

This code is slightly more complicated than I'd like to... :/ It's tricky to
support both making the main thread block and not block at the same time.

But, otoh, it's temporary, and luckily it's easy to make the tests run for both
configurations.

[marja, 2014-10-14 09:23:50]

And, in addition, the background is that jochen@ says making the main thread
block might be better for performance - since I observed in some cases that it's
difficult to get the main thread's attention back (it might be processing other
stuff, especially from other iframes). So we surely have it in notifyFinished(),
and it might be better not to lose it but wait there until parsing is done, and
move on (possibly compile, if the client wants so).

[haraken, 2014-10-14 14:05:17]

On 2014/10/14 09:23:50, marja wrote:
> And, in addition, the background is that jochen@ says making the main thread
> block might be better for performance - since I observed in some cases that
it's
> difficult to get the main thread's attention back (it might be processing
other
> stuff, especially from other iframes). So we surely have it in
notifyFinished(),
> and it might be better not to lose it but wait there until parsing is done,
and
> move on (possibly compile, if the client wants so).

Thanks for the background!

Actually I'm slightly negative to the idea of blocking the main thread while
parsing. Given that Blink is now trying to give the Blink scheduler a full
control of the main thread, it wouldn't be nice to try to control the main
thread in other places even if it improves performance of specific things.
Alternately, would it be possible to tell information to the Blink scheduler so
that the Blink scheduler can schedule the main thread for script execution more
wisely? It seems a right way to go IMO. What do you think?

[marja, 2014-10-14 14:22:52]

On 2014/10/14 14:05:17, haraken wrote:
> On 2014/10/14 09:23:50, marja wrote:
> > And, in addition, the background is that jochen@ says making the main thread
> > block might be better for performance - since I observed in some cases that
> it's
> > difficult to get the main thread's attention back (it might be processing
> other
> > stuff, especially from other iframes). So we surely have it in
> notifyFinished(),
> > and it might be better not to lose it but wait there until parsing is done,
> and
> > move on (possibly compile, if the client wants so).
> 
> Thanks for the background!
> 
> Actually I'm slightly negative to the idea of blocking the main thread while
> parsing. Given that Blink is now trying to give the Blink scheduler a full
> control of the main thread, it wouldn't be nice to try to control the main
> thread in other places even if it improves performance of specific things.
> Alternately, would it be possible to tell information to the Blink scheduler
so
> that the Blink scheduler can schedule the main thread for script execution
more
> wisely? It seems a right way to go IMO. What do you think?

Hmm, I don't know the scheduling code very closely...

The "problem" I was seeing in some experiments that the main thread picked up
some other work (mainly: working on a different iframe) since the script was
loaded but not yet parsed. Then parsing finished, but we had to wait for that
other work to complete before the main thread could again work on the script.
How would using the scheduler solve that problem? The problem was not that the
main thread doesn't get enough execution time, but that it picks up some other
work when it (supposedly) would be better to wait a bit and then work on the
script.

Anyhow, we're not sure whether this will be a performance improvement or not -
if it turns out it doesn't improve performance, then we would go with the
non-blocking solution.

[kouhei (in TOK), 2014-10-14 14:26:35]

kouhei@chromium.org changed reviewers:
+ kouhei@chromium.org

[kouhei (in TOK), 2014-10-14 14:26:35]

+Sami

I think scheduler should help here. Blink scheduler can prioritize certain task
over other tasks. It currently run compositor related tasks at higher priority.
Script parsing can do the same.

[marja, 2014-10-14 14:37:06]

On 2014/10/14 14:26:35, kouhei wrote:
> +Sami
> 
> I think scheduler should help here. Blink scheduler can prioritize certain
task
> over other tasks. It currently run compositor related tasks at higher
priority.
> Script parsing can do the same.

I'm not convinced yet. In this particular situation, the main thread doesn't
have a set of tasks to choose from (so that prioritizing them would help), but
the script is simply not yet ready, and the question is, is it beneficial for
the main thread to wait for it to become ready or not.

[Sami, 2014-10-15 13:05:43]

skyostil@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[Sami, 2014-10-15 13:11:50]

skyostil@chromium.org changed reviewers:
+ alexclarke@chromium.org, skyostil@chromium.org

[Sami, 2014-10-15 13:11:51]

I guess you're trying to improve page load time with this patch? I think I agree
that ultimately the scheduler is the right place to do this. The reason is that
by blocking here you might be delaying other main thread work (e.g.,
compositing), and whether we should prioritize loading time or animation
smoothness really depends on the larger state of the system.

We've been talking about adding some kind of load-time policy to the scheduler,
which would try to prioritize things like this. As a straw man, a possible
interface for this could be something like Scheduler::postLoadingTask(). If you
did that here, the scheduler could make sure your task is at the front of the
queue when appropriate.

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

https://codereview.chromium.org/651163002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:324: if (!isFinished()) {
Should this be a while loop to guard against spurious wakeups?

[Sami, 2014-10-15 13:13:10]

Forgot to ask, is there some specific thing on the main thread you're hoping to
jump ahead of with this change (you mentioned iframes?) or is it whatever work
that was already posted there?

[marja, 2014-10-15 13:21:03]

I still don't get how this can work w/ priorities. The main thread will pick up
whatever work is there, and if the parsing is not yet ready, what can we do?
What task exactly whould you prioritize to change this?

On 2014/10/15 13:13:10, Sami wrote:
> Forgot to ask, is there some specific thing on the main thread you're hoping
to
> jump ahead of with this change (you mentioned iframes?) or is it whatever work
> that was already posted there?

In my manual tests, it was e.g., running scripts which belong to some other
iframe on the page, but it's not specific; it can be anything.

The underlying assumption is that it's better for the main thread to wait on
parsing instead of doing other work (that is, the things we need to parse are on
the blocking path of giving the user a usable page).

Note that in the non-streaming code path, the main thread does the full parsing,
so it's going to block at least as long as this solution.

[marja, 2014-10-15 13:26:38]

Hmm, so one thing I should've probably mentioned explicitly is that in my
experiments, the main thread ended up picking extra work which took a long time,
so, overall, even if the parsing task finished during it, the main thread
couldn't start compiling the script because it was doing other things. So, even
if the main thread would prioritize compiling when the task is in the task
queue, it doesn't necessarily get executed much earlier... though, bypassing the
other tasks in the task queue might help.

[Sami, 2014-10-15 14:04:16]

On 2014/10/15 13:21:03, marja wrote:
> I still don't get how this can work w/ priorities. The main thread will pick
up
> whatever work is there, and if the parsing is not yet ready, what can we do?
> What task exactly whould you prioritize to change this?

If necessary, we could throttle every other task in the scheduler which isn't a
loading task. I'm not sure we need to go that far however. Just by throttling
the compositor, for instance, would free up some timeon the main thread.

This, of course, depends on getting those other things hooked up for the
scheduler in the first place. We can already prioritize over Blink timers and
stuff that comes from callOnMainThread, but there are still other things still
go directly to base::MessageLoop.

> On 2014/10/15 13:13:10, Sami wrote:
> > Forgot to ask, is there some specific thing on the main thread you're hoping
> to
> > jump ahead of with this change (you mentioned iframes?) or is it whatever
work
> > that was already posted there?
> 
> In my manual tests, it was e.g., running scripts which belong to some other
> iframe on the page, but it's not specific; it can be anything.
> 
> The underlying assumption is that it's better for the main thread to wait on
> parsing instead of doing other work (that is, the things we need to parse are
on
> the blocking path of giving the user a usable page).

Right. I wonder if there is a way to turn that heuristic into code? For
compositing work we're currently relying on signals like having recently seen
input events.

> Note that in the non-streaming code path, the main thread does the full
parsing,
> so it's going to block at least as long as this solution.

I see. Still potentially stealing our precious compositing time then :)

> Hmm, so one thing I should've probably mentioned explicitly is that in my
> experiments, the main thread ended up picking extra work which took a long
time,
> so, overall, even if the parsing task finished during it, the main thread
> couldn't start compiling the script because it was doing other things. So,
even
> if the main thread would prioritize compiling when the task is in the task
> queue, it doesn't necessarily get executed much earlier... though, bypassing
the
> other tasks in the task queue might help.

We've seen similar problems elsewhere. The current plan is to either try and
split those huge things into smaller iterable chunks or add yielding logic which
aborts the long running thing if something more urgent is pending. Obviously
that won't work for everything, especially user JS.

[marja, 2014-10-23 08:25:39]

Hi, sorry for delay, let's re-ignite the discussions here.

First, the state of art now:
- Streaming is behind a Finch experiment, not enabled by default. It's possible
to demonstrate with hand-picked pages that streaming is a win for page load
time, but Finch data shows that just streaming everything like it's done now is
a small loss. So, something needs to be fixed, otherwise I cannot enable
streaming for the whole population.
- In the default non-streaming mode, the main thread blocks to parse and compile
every script. So that's our baseline.
- One idea about how to get rid of the regression is to make the main thread
wait for the streamer thread when all data has arrived. We don't know whether
that's a good thing or not, before enabling it and seeing what Finch says. If it
turns out to be a good thing, it's still *less* blocking than in the baseline
case. I think it's a reasonable thing to investigate.
- The worst case scenario is that I won't be able to enable streaming at all, in
which case, we stay in the baseline case and the main thread always blocks for
parsing and compiling.
- There's upcoming scheduler work, but looks like getting this work smoothly
would mean offloading more work from the main thread, or cutting it into smaller
pieces, and we cannot do all of that now (especially, script parsing / compiling
/ running is done in the main thread).

To this end, I plan to run the following Finch experiments (implemented by this
CL):
1) stream only async and deferred scripts, don't block
2) stream all scripts, don't block
3) stream all scripts, block the main thread for parsing blocking scripts. The
rationale is that a) before the parsing blocking script is loaded, we probably
don't care about other work done by the main thread, such as compositing b) it's
still less blocking than the baseline case.

For the hand-picked pages, it's enough to stream async and deferred script, so,
hopefully 1 or 2 turns out the be the best version to enable, and we don't need
to bother with blocking the main thread.

If 3 turns out to be the best version, then I can enable streaming with that
version, and experiment with scheduler-related solutions as a follow up,
comparing them against version 3 with Finch.

If all the versions are still a regression, then it's back to the drawing board.

jochen@ and haraken@ said this LGTT, but haraken@ wanted skyostil@ to tune in.
Do you think this experiment is reasonable? Of course I'd also like to block the
main thread as little as possible, but taking into account that the baseline
here is very much blocking, I think it's reasonable to experiment with this.

[marja, 2014-10-23 08:27:08]

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

[rmcilroy, 2014-10-23 09:10:30]

On 2014/10/23 08:25:39, marja wrote:
> Hi, sorry for delay, let's re-ignite the discussions here.
> 
> First, the state of art now:
> - Streaming is behind a Finch experiment, not enabled by default. It's
possible
> to demonstrate with hand-picked pages that streaming is a win for page load
> time, but Finch data shows that just streaming everything like it's done now
is
> a small loss. So, something needs to be fixed, otherwise I cannot enable
> streaming for the whole population.
> - In the default non-streaming mode, the main thread blocks to parse and
compile
> every script. So that's our baseline.
> - One idea about how to get rid of the regression is to make the main thread
> wait for the streamer thread when all data has arrived. We don't know whether
> that's a good thing or not, before enabling it and seeing what Finch says. If
it
> turns out to be a good thing, it's still *less* blocking than in the baseline
> case. I think it's a reasonable thing to investigate.
> - The worst case scenario is that I won't be able to enable streaming at all,
in
> which case, we stay in the baseline case and the main thread always blocks for
> parsing and compiling.
> - There's upcoming scheduler work, but looks like getting this work smoothly
> would mean offloading more work from the main thread, or cutting it into
smaller
> pieces, and we cannot do all of that now (especially, script parsing /
compiling
> / running is done in the main thread).
> 
> To this end, I plan to run the following Finch experiments (implemented by
this
> CL):
> 1) stream only async and deferred scripts, don't block
> 2) stream all scripts, don't block
> 3) stream all scripts, block the main thread for parsing blocking scripts. The
> rationale is that a) before the parsing blocking script is loaded, we probably
> don't care about other work done by the main thread, such as compositing b)
it's
> still less blocking than the baseline case.
> 
> For the hand-picked pages, it's enough to stream async and deferred script,
so,
> hopefully 1 or 2 turns out the be the best version to enable, and we don't
need
> to bother with blocking the main thread.
> 
> If 3 turns out to be the best version, then I can enable streaming with that
> version, and experiment with scheduler-related solutions as a follow up,
> comparing them against version 3 with Finch.
> 
> If all the versions are still a regression, then it's back to the drawing
board.
> 
> jochen@ and haraken@ said this LGTT, but haraken@ wanted skyostil@ to tune in.
> Do you think this experiment is reasonable? Of course I'd also like to block
the
> main thread as little as possible, but taking into account that the baseline
> here is very much blocking, I think it's reasonable to experiment with this.

From my point of view I think it is reasonable to do this experiment. As Marija
says this isn't worse than the current blocking, and allows us to get some
numbers which could help guide how we prioritise script parsing when loading.

I still think that the end goal should be to remove the blocking and instead
post a task which is treated as high priority by the scheduler during loading,
such that it gets run before any compositor tasks or other normal tasks and
triggers any currently long running task to yield via the shouldYield function.
In this world we may also want to tweak the priority of the script streaming
background thread in notifyFinished to ensure it gets scheduled  whenever it has
data in preference to anything on the main thread. This would hopefully not
regress script parsing performance while allowing work to continue on the main
thread.  However, all this depends on us implementing the loading policy in the
scheduler so I would be fine with this landing until then.

[rmcilroy, 2014-10-23 09:27:58]

On 2014/10/23 09:10:30, rmcilroy wrote:
> On 2014/10/23 08:25:39, marja wrote:
> > Hi, sorry for delay, let's re-ignite the discussions here.
> > 
> > First, the state of art now:
> > - Streaming is behind a Finch experiment, not enabled by default. It's
> possible
> > to demonstrate with hand-picked pages that streaming is a win for page load
> > time, but Finch data shows that just streaming everything like it's done now
> is
> > a small loss. So, something needs to be fixed, otherwise I cannot enable
> > streaming for the whole population.
> > - In the default non-streaming mode, the main thread blocks to parse and
> compile
> > every script. So that's our baseline.
> > - One idea about how to get rid of the regression is to make the main thread
> > wait for the streamer thread when all data has arrived. We don't know
whether
> > that's a good thing or not, before enabling it and seeing what Finch says.
If
> it
> > turns out to be a good thing, it's still *less* blocking than in the
baseline
> > case. I think it's a reasonable thing to investigate.
> > - The worst case scenario is that I won't be able to enable streaming at
all,
> in
> > which case, we stay in the baseline case and the main thread always blocks
for
> > parsing and compiling.
> > - There's upcoming scheduler work, but looks like getting this work smoothly
> > would mean offloading more work from the main thread, or cutting it into
> smaller
> > pieces, and we cannot do all of that now (especially, script parsing /
> compiling
> > / running is done in the main thread).
> > 
> > To this end, I plan to run the following Finch experiments (implemented by
> this
> > CL):
> > 1) stream only async and deferred scripts, don't block
> > 2) stream all scripts, don't block
> > 3) stream all scripts, block the main thread for parsing blocking scripts.
The
> > rationale is that a) before the parsing blocking script is loaded, we
probably
> > don't care about other work done by the main thread, such as compositing b)
> it's
> > still less blocking than the baseline case.
> > 
> > For the hand-picked pages, it's enough to stream async and deferred script,
> so,
> > hopefully 1 or 2 turns out the be the best version to enable, and we don't
> need
> > to bother with blocking the main thread.
> > 
> > If 3 turns out to be the best version, then I can enable streaming with that
> > version, and experiment with scheduler-related solutions as a follow up,
> > comparing them against version 3 with Finch.
> > 
> > If all the versions are still a regression, then it's back to the drawing
> board.
> > 
> > jochen@ and haraken@ said this LGTT, but haraken@ wanted skyostil@ to tune
in.
> > Do you think this experiment is reasonable? Of course I'd also like to block
> the
> > main thread as little as possible, but taking into account that the baseline
> > here is very much blocking, I think it's reasonable to experiment with this.
> 
> From my point of view I think it is reasonable to do this experiment. As
Marija
> says this isn't worse than the current blocking, and allows us to get some
> numbers which could help guide how we prioritise script parsing when loading.
> 
> I still think that the end goal should be to remove the blocking and instead
> post a task which is treated as high priority by the scheduler during loading,
> such that it gets run before any compositor tasks or other normal tasks and
> triggers any currently long running task to yield via the shouldYield
function.
> In this world we may also want to tweak the priority of the script streaming
> background thread in notifyFinished to ensure it gets scheduled  whenever it
has
> data in preference to anything on the main thread. This would hopefully not
> regress script parsing performance while allowing work to continue on the main
> thread.  However, all this depends on us implementing the loading policy in
the
> scheduler so I would be fine with this landing until then.

Plus, any project which can "allow for killing 10% more zombies" [1] has got to
continue in order to save us from the coming zombie apocalypses ;). 

[1]
https://groups.google.com/a/google.com/d/msg/v8-team-muc/wIL4LnBVMxE/nQuDYcdI...

[jochen (gone - plz use gerrit), 2014-10-23 09:40:59]

lgtm with nit

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

https://codereview.chromium.org/651163002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.h:99: ScriptStreamer(ScriptResource*,
v8::ScriptCompiler::StreamedSource::Encoding, PendingScript::Type, bool
blockMainThreadAfterLoading);
nit. enum instead of bool maybe?

[Sami, 2014-10-23 12:09:13]

I agree with Ross that this seems like a good first step. One thing I'd like to
ask is trace event annotation so that it's super obvious from a trace when the
main thread is blocking (if that isn't the case already). As a bonus if that
time ends up being substantial it will show up automatically in the new
task_execution_time benchmark: 

https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=android-nexus...

> 3) stream all scripts, block the main thread for parsing blocking scripts. The
> rationale is that a) before the parsing blocking script is loaded, we probably
> don't care about other work done by the main thread, such as compositing

I don't know enough to say if this is a valid assumption or not, but I'd love to
find out. I wonder if we'd want more fine grained conditions such as whether the
user is trying to scroll or not.

[marja, 2014-10-23 14:11:45]

Thanks for review!

Re: adding a trace event: Yes, that's a good idea, so added
v8.mainThreadWaitingForParserThread around the wait.

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

https://codereview.chromium.org/651163002/diff/40001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.h:99: ScriptStreamer(ScriptResource*,
v8::ScriptCompiler::StreamedSource::Encoding, PendingScript::Type, bool
blockMainThreadAfterLoading);
On 2014/10/23 09:40:59, jochen wrote:
> nit. enum instead of bool maybe?

Done: Passing the mode here (as discussed).

[marja, 2014-10-23 14:12:39]

The CQ bit was checked by marja@chromium.org

[commit-bot: I haz the power, 2014-10-23 14:13:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/651163002/60001

[commit-bot: I haz the power, 2014-10-23 15:23:40]

Committed patchset #4 (id:60001) as 184277

[haraken, 2014-10-23 15:32:10]

LGTM

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

https://codereview.chromium.org/651163002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:231: MutexLocker locker(m_mutex);

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

https://codereview.chromium.org/651163002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:258: MutexLocker locker(m_mutex);

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

https://codereview.chromium.org/651163002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:334: if (shouldBlockMainThread() &&
m_parsingFinished) {

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

https://codereview.chromium.org/651163002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:395: MutexLocker locker(m_mutex);

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

https://codereview.chromium.org/651163002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptStreamingMode.h (right):

https://codereview.chromium.org/651163002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamingMode.h:13: enum ScriptStreamingMode {

Nit: I'd put this enum into ScriptStreaming.h.

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

Thanks for comments (skyostil@, I also overlooked one code comment from you, I'm
answering it here).

This was too broken, so I reverted it and created a fixed version in
https://codereview.chromium.org/674133002/ . haraken@, your comments are
addressed there.

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

https://codereview.chromium.org/651163002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:324: if (!isFinished()) {
On 2014/10/15 13:11:51, Sami wrote:
> Should this be a while loop to guard against spurious wakeups?

Hmm, dunno, it's a bit of overly defensive programming; m_loadingFinished is set
to true here, and m_parsingFinished is set to true before the cond var is
signaled, so, there shouldn't be other wakeups.

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

https://codereview.chromium.org/651163002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:231: MutexLocker locker(m_mutex);
On 2014/10/23 15:32:09, haraken wrote:
> 
> Let's add ASSERT(!isFinished()).

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.

https://codereview.chromium.org/651163002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:258: MutexLocker locker(m_mutex);
On 2014/10/23 15:32:09, haraken wrote:
> 
> This lock is a bit too wide. Shall we add the MutexLocker into isFinished()
and
> use ASSERT(!isFinished()) here?

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.

https://codereview.chromium.org/651163002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:334: if (shouldBlockMainThread() &&
m_parsingFinished) {
On 2014/10/23 15:32:09, haraken wrote:
> 
> Do we need the '&& m_parsingFinished' check? I guess it is always true.
> 

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.

https://codereview.chromium.org/651163002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:395: MutexLocker locker(m_mutex);
On 2014/10/23 15:32:09, haraken wrote:
> 
> This lock is too wide. We don't need to hold a lock while calling
> notifyFinished(). We can add the MutexLocker to isFinished().
> 

Made this code release the lock before calling client.

https://codereview.chromium.org/651163002/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/ScriptStreamingMode.h (right):

https://codereview.chromium.org/651163002/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamingMode.h:13: enum ScriptStreamingMode {
On 2014/10/23 15:32:10, haraken wrote:
> 
> Nit: I'd put this enum into ScriptStreaming.h.

I'd rather keep it here, this is the minimal file to be included by Settings.h.
I don't want to include the whole ScriptStreamer.h there, it's unnecessary.

[Sami, 2014-10-24 13:15:35]

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

https://codereview.chromium.org/651163002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:324: if (!isFinished()) {
On 2014/10/24 13:04:40, marja wrote:
> On 2014/10/15 13:11:51, Sami wrote:
> > Should this be a while loop to guard against spurious wakeups?
> 
> Hmm, dunno, it's a bit of overly defensive programming; m_loadingFinished is
set
> to true here, and m_parsingFinished is set to true before the cond var is
> signaled, so, there shouldn't be other wakeups.

I think we're using pthread_cond_wait under the hood, which can sometimes return
without being signaled because of the way it's implemented. I'd suggest making
this loop to be safe.

See here:
https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...

[marja, 2014-10-24 13:19:29]

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

https://codereview.chromium.org/651163002/diff/20001/Source/bindings/core/v8/...
Source/bindings/core/v8/ScriptStreamer.cpp:324: if (!isFinished()) {
On 2014/10/24 13:15:35, Sami wrote:
> On 2014/10/24 13:04:40, marja wrote:
> > On 2014/10/15 13:11:51, Sami wrote:
> > > Should this be a while loop to guard against spurious wakeups?
> > 
> > Hmm, dunno, it's a bit of overly defensive programming; m_loadingFinished is
> set
> > to true here, and m_parsingFinished is set to true before the cond var is
> > signaled, so, there shouldn't be other wakeups.
> 
> I think we're using pthread_cond_wait under the hood, which can sometimes
return
> without being signaled because of the way it's implemented. I'd suggest making
> this loop to be safe.
> 
> See here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...

Oh wow. I wasn't aware of that. TIL. Fixed in the new version.

[yesdear24344_gmail.com, 2016-04-28 22:53:16]

On Tuesday, October 14, 2014 at 2:22:10 AM UTC-7, ma...@chromium.org wrote:
> Reviewers: haraken,
> 
> Message:
> haraken, ptal.
> 
> This code is slightly more complicated than I'd like to... :/ It's tricky to
> support both making the main thread block and not block at the same time.
> 
> But, otoh, it's temporary, and luckily it's easy to make the tests run for  
> both
> configurations.
> 
> Description:
> Script streaming: Add an option to make the main thread block (wait for  
> parsing)
> 
> This makes the main thread block and wait for the parser thread when all the
> script data has arrived. The goal is to ensure that the script (which can  
> now be
> compiled) will get the main thread's attention as soon as possible.
> 
> This feature is temporary: The usefulness of blocking will be evaluated with
> Finch, and this option will be removed (blocking will be always enabled or  
> never
> enabled, based on which option turns out to be better).
> 
> R=haraken@chromium.org
> BUG=
> 
> Please review this at https://codereview.chromium.org/651163002/
> 
> Base URL: https://chromium.googlesource.com/chromium/blink.git@master
> 
> Affected files (+94, -40 lines):
>    M Source/bindings/core/v8/ScriptStreamer.h
>    M Source/bindings/core/v8/ScriptStreamer.cpp
>    M Source/bindings/core/v8/ScriptStreamerTest.cpp
>    M Source/bindings/core/v8/ScriptStreamerThread.h
>    M Source/bindings/core/v8/ScriptStreamerThread.cpp
>    M Source/core/frame/Settings.in

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[yesdear24344_gmail.com, 2016-04-28 22:55:05]

What.the hell rc ? This is oc :(

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.