[compiler-dispatcher] make it so that we can always parse on bg threads

[compiler-dispatcher] make it so that we can always parse on bg threads

BUG=v8:5215
R=rmcilroy@chromium.org,hpayer@chromium.org,vogelheim@chromium.org,marja@chromium.org

Review-Url: https://codereview.chromium.org/2625413004
Cr-Commit-Position: refs/heads/master@{#42361}
Committed: https://chromium.googlesource.com/v8/v8/+/f40fdd1f3be26cac6f15e3849ff2acd8e86a5772

[jochen (gone - plz use gerrit), 2017-01-13 13:19:05]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 13:19:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2017-01-13 13:21:40]

Daniel, please take a look at the entire CL

Hannes, I need your feedback specifically on the one block I pointed out

Marja/Ross: fyi

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
File src/compiler-dispatcher/compiler-dispatcher-job.cc (right):

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher-job.cc:124: if
(isolate_->heap()->lo_space()->Contains(*source)) {
Hannes, ptal at this if () {} block: here I'm depending on the fact that strings
in LO space won't move, so I can parse off of them on bg threads.

[commit-bot: I haz the power, 2017-01-13 13:55:17]

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

[commit-bot: I haz the power, 2017-01-13 13:55:18]

Dry run: This issue passed the CQ dry run.

[Hannes Payer (out of office), 2017-01-13 14:01:04]

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
File src/compiler-dispatcher/compiler-dispatcher-job.cc (right):

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher-job.cc:124: if
(isolate_->heap()->lo_space()->Contains(*source)) {
On 2017/01/13 13:21:39, jochen wrote:
> Hannes, ptal at this if () {} block: here I'm depending on the fact that
strings
> in LO space won't move, so I can parse off of them on bg threads.

Correct, LO don't move.

[vogelheim, 2017-01-13 15:20:22]

I think the New/delete thing is serious (but maybe I overlooked something.) All
other stuff is optional.

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
File src/compiler-dispatcher/compiler-dispatcher-job.cc (right):

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher-job.cc:117: source =
String::Flatten(source);
[all below:]

I'm vaguely uncomfortable with this whole logic, since it (potentially)
introduces a new copy, while we already have so many copies of the source string
that we're desperately trying to get rid of some.

I take it this should rarely (if ever) be triggered for 'real' code, since
pretty much anything we get from Blink is an External(One|Two)ByteString? If so,
my concern is still there, but more academic.


Since at the current state I don't really have an alternative I don#t want to
hold this up. I do suspect we can do better by looking at, and I do think we
need to revisit this at some point...

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher-job.cc:143: data =
zone_->New(byte_len);
Who will delete this?

As I see it, it get pointed to from an External(One|Two)ByteString, but I
thought those typically don't delete their memory (as they don't normally own
it), so... where does this get deleted?

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
File src/compiler-dispatcher/compiler-dispatcher-job.h (right):

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher-job.h:10: #include "include/v8.h"
Can you forward declare v8::String::ExternalStringResourceBase?

(Not sure, actually, with it being an inner class and C++ being weird...)

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
File src/compiler-dispatcher/compiler-dispatcher.cc (right):

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher.cc:71: return (job->status() ==
CompileJobStatus::kReadyToParse) ||
style nitpick: parentheses aren't really needed. See IsFinished above.

[rmcilroy, 2017-01-13 15:56:22]

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
File src/compiler-dispatcher/compiler-dispatcher-job.cc (right):

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher-job.cc:124: if
(isolate_->heap()->lo_space()->Contains(*source)) {
On 2017/01/13 14:01:04, Hannes Payer wrote:
> On 2017/01/13 13:21:39, jochen wrote:
> > Hannes, ptal at this if () {} block: here I'm depending on the fact that
> strings
> > in LO space won't move, so I can parse off of them on bg threads.
> 
> Correct, LO don't move.

Is it common for the source to be in the LO space? I'm guessing the source is
usually an external string, and would only be on the heap for eval functions (am
I missing other situations?), and therefore would only be in the LO space if
it's a 4KB eval - I'm guessing this isn't super common - is it worth the added
complexity here?

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
File src/compiler-dispatcher/compiler-dispatcher.cc (right):

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher.cc:72: (job->status() ==
CompileJobStatus::kReadyToCompile);
This now implicitly assumes the compiler dispatcher only compiles Ignition
bytecode. Could we DCHECK this when enabling the dispatcher?

[jochen (gone - plz use gerrit), 2017-01-13 16:14:40]

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
File src/compiler-dispatcher/compiler-dispatcher-job.cc (right):

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher-job.cc:117: source =
String::Flatten(source);
On 2017/01/13 at 15:20:22, vogelheim wrote:
> [all below:]
> 
> I'm vaguely uncomfortable with this whole logic, since it (potentially)
introduces a new copy, while we already have so many copies of the source string
that we're desperately trying to get rid of some.
> 
> I take it this should rarely (if ever) be triggered for 'real' code, since
pretty much anything we get from Blink is an External(One|Two)ByteString? If so,
my concern is still there, but more academic.
> 
> 
> Since at the current state I don't really have an alternative I don#t want to
hold this up. I do suspect we can do better by looking at, and I do think we
need to revisit this at some point...

Right. I'm also trying to reduce the impact by just copying the function, not
the entire string (if I have to copy)

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher-job.cc:124: if
(isolate_->heap()->lo_space()->Contains(*source)) {
On 2017/01/13 at 15:56:22, rmcilroy wrote:
> On 2017/01/13 14:01:04, Hannes Payer wrote:
> > On 2017/01/13 13:21:39, jochen wrote:
> > > Hannes, ptal at this if () {} block: here I'm depending on the fact that
> > strings
> > > in LO space won't move, so I can parse off of them on bg threads.
> > 
> > Correct, LO don't move.
> 
> Is it common for the source to be in the LO space? I'm guessing the source is
usually an external string, and would only be on the heap for eval functions (am
I missing other situations?), and therefore would only be in the LO space if
it's a 4KB eval - I'm guessing this isn't super common - is it worth the added
complexity here?

no, I expect most cases to be external strings, or small evals. However, if
somebody implements a module loading using eval, we might hit this.

I think it's preferable to just always be able to run on bg threads

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher-job.cc:143: data =
zone_->New(byte_len);
On 2017/01/13 at 15:20:22, vogelheim wrote:
> Who will delete this?
> 
> As I see it, it get pointed to from an External(One|Two)ByteString, but I
thought those typically don't delete their memory (as they don't normally own
it), so... where does this get deleted?

the zone_ will delete this

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
File src/compiler-dispatcher/compiler-dispatcher-job.h (right):

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher-job.h:10: #include "include/v8.h"
On 2017/01/13 at 15:20:22, vogelheim wrote:
> Can you forward declare v8::String::ExternalStringResourceBase?
> 
> (Not sure, actually, with it being an inner class and C++ being weird...)

yeah, that's not possible :(

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
File src/compiler-dispatcher/compiler-dispatcher.cc (right):

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher.cc:71: return (job->status() ==
CompileJobStatus::kReadyToParse) ||
On 2017/01/13 at 15:20:22, vogelheim wrote:
> style nitpick: parentheses aren't really needed. See IsFinished above.

done

https://codereview.chromium.org/2625413004/diff/1/src/compiler-dispatcher/com...
src/compiler-dispatcher/compiler-dispatcher.cc:72: (job->status() ==
CompileJobStatus::kReadyToCompile);
On 2017/01/13 at 15:56:22, rmcilroy wrote:
> This now implicitly assumes the compiler dispatcher only compiles Ignition
bytecode. Could we DCHECK this when enabling the dispatcher?

done

[jochen (gone - plz use gerrit), 2017-01-13 16:14:47]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 16:14:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 17:04:58]

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

[commit-bot: I haz the power, 2017-01-13 17:04:59]

Dry run: This issue passed the CQ dry run.

[vogelheim, 2017-01-16 10:17:22]

lgtm

[jochen (gone - plz use gerrit), 2017-01-16 10:17:51]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2017-01-16 10:17:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[marja, 2017-01-16 10:20:36]

(late lgtm)

[commit-bot: I haz the power, 2017-01-16 10:49:17]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484561871559470,
"parent_rev": "c6f0de8dd63a58c7a96c774ec43c7cdd4ad78e7c", "commit_rev":
"f40fdd1f3be26cac6f15e3849ff2acd8e86a5772"}

[commit-bot: I haz the power, 2017-01-16 10:49:21]

Description was changed from

==========
[compiler-dispatcher] make it so that we can always parse on bg threads

BUG=v8:5215
R=rmcilroy@chromium.org,hpayer@chromium.org,vogelheim@chromium.org,marja@chro...
==========

to

==========
[compiler-dispatcher] make it so that we can always parse on bg threads

BUG=v8:5215
R=rmcilroy@chromium.org,hpayer@chromium.org,vogelheim@chromium.org,marja@chro...

Review-Url: https://codereview.chromium.org/2625413004
Cr-Commit-Position: refs/heads/master@{#42361}
Committed:
https://chromium.googlesource.com/v8/v8/+/f40fdd1f3be26cac6f15e3849ff2acd8e86...
==========

[commit-bot: I haz the power, 2017-01-16 10:49:22]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/f40fdd1f3be26cac6f15e3849ff2acd8e86...