Two-threaded parser

Two-threaded parser

This is how it works:
- The background thread runs PreParser to find lazy functions. The main thread
  runs Parser, and uses the data produced by the background thread to skip over
  lazy functions.

- This can only work for external strings, since they're stored outside the V8
  heap. ExternalTwoByteStringUtf16CharacterStream and
  ExternalOneByteStringUtf16CharacterStream keep a Handle<String> but they are
  safe to use in the background thread, since they don't access the Handle after
  the constructor.

- The "cached data" is still produced by Parser like before, and stored like
  before.

BUG=

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

svenpanne, ptal, {dcarney,ulan,dslomov,mstarzinger}, you might also want to have
a look.

Try jobs are in patch set 25.

[Sven Panne, 2014-04-15 11:21:30]

I only had a quick look so far, but I have already one suggestion: Instead of
describing verbosely why passing a Handle<ExternalFooBarString> is safe, just
pass a pointer to the raw data. This makes it obvious that the contract holds
without reading any kind of (potentially lying ;-) comments. Moving a single
call to all call sites is a very good trade-off in our case.

[marja, 2014-04-15 12:08:09]

On 2014/04/15 11:21:30, Sven Panne wrote:
> I only had a quick look so far, but I have already one suggestion: Instead of
> describing verbosely why passing a Handle<ExternalFooBarString> is safe, just
> pass a pointer to the raw data. This makes it obvious that the contract holds
> without reading any kind of (potentially lying ;-) comments. Moving a single
> call to all call sites is a very good trade-off in our case.

OK, did that (see latest patch set). The downside is that the caller needs to
keep the memory alive (the Handle used to take care of that), which is slightly
ugly, but not a problem for the call sites there are now.

[Sven Panne, 2014-04-16 11:55:35]

https://codereview.chromium.org/214883002/diff/530001/src/parser-thread.cc
File src/parser-thread.cc (right):

https://codereview.chromium.org/214883002/diff/530001/src/parser-thread.cc#ne...
src/parser-thread.cc:60: mutex_.Lock();
Use LockGuard here...

https://codereview.chromium.org/214883002/diff/530001/src/parser-thread.cc#ne...
src/parser-thread.cc:70: mutex_.Lock();
... and here.

https://codereview.chromium.org/214883002/diff/530001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/214883002/diff/530001/src/parser.cc#newcode898
src/parser.cc:898: new ExternalOneByteStringUtf16CharacterStream(
Extract the creation of the Utf16CharacterStream* out into a separate function
(maybe returning NULL when it is not an ASCII/TwoByte string). If it returns
non-NULL, we do exactly the same thing, something which is not very obvious
right now and furthermore makes half of the comment above superfluous (or at
least this part should be moved as well).

https://codereview.chromium.org/214883002/diff/530001/src/parser.cc#newcode953
src/parser.cc:953: if (thread_) {
Lift this block out into a separate method, it's used identically below.

https://codereview.chromium.org/214883002/diff/530001/src/parser.cc#newcode3542
src/parser.cc:3542: } else if (thread_) {
Hmmm, the block below contains some heavy copy-n-paste from the code above, can
we somehow reduce this?

[marja, 2014-04-16 14:33:09]

Thanks for comments!

Relaying some offline code review discussions: mstarzinger@ suggested adding a
flag to disable parallel parsing altogether, and I did that.

https://codereview.chromium.org/214883002/diff/530001/src/parser-thread.cc
File src/parser-thread.cc (right):

https://codereview.chromium.org/214883002/diff/530001/src/parser-thread.cc#ne...
src/parser-thread.cc:60: mutex_.Lock();
On 2014/04/16 11:55:35, Sven Panne wrote:
> Use LockGuard here...

Done.

https://codereview.chromium.org/214883002/diff/530001/src/parser-thread.cc#ne...
src/parser-thread.cc:70: mutex_.Lock();
On 2014/04/16 11:55:35, Sven Panne wrote:
> ... and here.

Done.

https://codereview.chromium.org/214883002/diff/530001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/214883002/diff/530001/src/parser.cc#newcode898
src/parser.cc:898: new ExternalOneByteStringUtf16CharacterStream(
On 2014/04/16 11:55:35, Sven Panne wrote:
> Extract the creation of the Utf16CharacterStream* out into a separate function
> (maybe returning NULL when it is not an ASCII/TwoByte string). If it returns
> non-NULL, we do exactly the same thing, something which is not very obvious
> right now and furthermore makes half of the comment above superfluous (or at
> least this part should be moved as well).

Extracted the Stream creation. But I don't get what you mean by "we do exactly
the same thing, something which is not very obvious right now and furthermore
makes half of the comment above superfluous (or  at least this part should be
moved as well)."

https://codereview.chromium.org/214883002/diff/530001/src/parser.cc#newcode953
src/parser.cc:953: if (thread_) {
On 2014/04/16 11:55:35, Sven Panne wrote:
> Lift this block out into a separate method, it's used identically below.

Done, and I also added a function for (possibly) starting the thread, where the
above thread-starting code goes.

https://codereview.chromium.org/214883002/diff/530001/src/parser.cc#newcode3542
src/parser.cc:3542: } else if (thread_) {
On 2014/04/16 11:55:35, Sven Panne wrote:
> Hmmm, the block below contains some heavy copy-n-paste from the code above,
can
> we somehow reduce this?

Done.

[marja, 2014-04-17 15:17:20]

svenpanne, could you take another look?

Some more offline code review: jochen@ said I should use the same background
thread for all tasks instead of spawning several. That is done in the latest
patch set.

[Michael Starzinger, 2014-04-23 09:07:10]

LGTM with one comment. In general I am not a big fan of "transferring ownership"
of C-allocated memory, that almost always goes wrong (like in this case I have
the feeling). Would it be possible to embed the external string stream into the
parsing job somehow?

https://codereview.chromium.org/214883002/diff/690001/src/parser-thread.cc
File src/parser-thread.cc (right):

https://codereview.chromium.org/214883002/diff/690001/src/parser-thread.cc#ne...
src/parser-thread.cc:167: delete current_task_;
This deletes the background parsing task, but I cannot find the place where the
external string stream is deleted. Is it missing?

https://codereview.chromium.org/214883002/diff/690001/src/preparse-data.h
File src/preparse-data.h (right):

https://codereview.chromium.org/214883002/diff/690001/src/preparse-data.h#new...
src/preparse-data.h:34: #include "preparse-data-format.h"
nit: Is it possible to alpha-sort includes here?

[ulan, 2014-04-23 10:18:20]

One suggestion, maybe for subsequent CL:

https://codereview.chromium.org/214883002/diff/690001/src/parser-thread.cc
File src/parser-thread.cc (right):

https://codereview.chromium.org/214883002/diff/690001/src/parser-thread.cc#ne...
src/parser-thread.cc:94: preparser.set_allow_lazy(true);
Would it be worthwhile to put these flags into a separate struct that can be
copied? Then this code would look like
params_.allow_lazy = true;
preparser.set_params(params_);

In other places you would need to pass one struct instead of passing each flag
indiviually.

[Sven Panne, 2014-04-23 10:36:28]

https://codereview.chromium.org/214883002/diff/690001/src/parser-thread.cc
File src/parser-thread.cc (right):

https://codereview.chromium.org/214883002/diff/690001/src/parser-thread.cc#ne...
src/parser-thread.cc:94: preparser.set_allow_lazy(true);
On 2014/04/23 10:18:20, ulan wrote:
> Would it be worthwhile to put these flags into a separate struct that can be
> copied? Then this code would look like
> params_.allow_lazy = true;
> preparser.set_params(params_);
> 
> In other places you would need to pass one struct instead of passing each flag
> indiviually.

+1 for this proposal, see
http://refactoring.com/catalog/introduceParameterObject.html

[Sven Panne, 2014-06-13 11:14:05]

What's the state of this CL?

[marja, 2014-06-13 11:20:05]

On 2014/06/13 11:14:05, Sven Panne wrote:
> What's the state of this CL?

On hold; the new best bet is to parse on a background thread while loading (for
that, https://codereview.chromium.org/314603004/ is the first step). After that
is done, this one might get resurrected (so we'd use several background
threads), but it's pretty questionable. At the moment I'm not doing anything
with this CL.

I'll remove reviewers so that this doesn't show up on your lists... (for future
reference, the list was mstarzinger@, ulan@, svenpanne@).