Add a basic compiler dispatcher

Add a basic compiler dispatcher

It doesn't schedule steps yet, but there are tests.

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

Committed: https://crrev.com/1bdf908db0512f65eb10c6b167f2d108cc06b673
Cr-Commit-Position: refs/heads/master@{#41650}

[jochen (gone - plz use gerrit), 2016-12-09 14:46:09]

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

[commit-bot: I haz the power, 2016-12-09 14:46:11]

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, 2016-12-09 15:15:51]

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

[commit-bot: I haz the power, 2016-12-09 15:15:52]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-12-12 08:07:27]

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

[commit-bot: I haz the power, 2016-12-12 08:07:33]

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, 2016-12-12 08:35:44]

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

[commit-bot: I haz the power, 2016-12-12 08:35:45]

Dry run: This issue passed the CQ dry run.

[marja, 2016-12-12 09:00:02]

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
File src/compiler-dispatcher/compiler-dispatcher-job.h (right):

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher-job.h:60: bool
IsProcessing(Handle<SharedFunctionInfo> shared) const;
Naming nit: IsProcessing has a connotation that something is "in progress", and
it's not clear that IsProcessing is true after the compilation job has finished
too.

IsAssociatedWith?

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
File src/compiler-dispatcher/compiler-dispatcher.cc (right):

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:24:
std::unique_ptr<CompilerDispatcherJob> job(new CompilerDispatcherJob(
Nit: no need to construct job if we do the early return based on the key. (But
this comment is hopefully obsolete bc of the next comment.)

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:29: // enqueued, we might not be
able to enqueue it.
Hmm, this is weird. So we cannot enqueue things because we're not able to assign
unique IDs for them?

So we need to store the jobs so that they're searchable by keys... but is there
anything preventing us from having multiple jobs with the same key, if we have a
data structure that supports that? E.g., do we always have the
SharedFunctionInfo available that we can check for real equality (not just
equality of the IDs) when we need to.

(I guess we need to check the real equality anyway, since the IDs are not
unique, so if we later check stuff like "is this function in the queue", it's
not enough to check based on IDs. So it should work.)

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:46: std::pair<int, int>
key(Script::cast(function->script())->id(),
How about having a job-finding helper function - it's confusing that the
ID-based check is not enough? Also, this code does find twice (first in
IsEnqueued and then here), that's unnecessary.

(In the version I suggest a helper function would also make it clearer.)

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:63: if (!IsEnqueued(function))
return;
Ditto

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
File src/compiler-dispatcher/compiler-dispatcher.h (right):

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.h:50: static bool
DoNextStepOnMainThread(CompilerDispatcherJob* job);
Afaics these don't need to be in the header, they can be in an unnamed namespace
in the cc file.

[marja, 2016-12-12 09:02:08]

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
File src/compiler-dispatcher/compiler-dispatcher.h (right):

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.h:50: static bool
DoNextStepOnMainThread(CompilerDispatcherJob* job);
On 2016/12/12 09:00:02, marja wrote:
> Afaics these don't need to be in the header, they can be in an unnamed
namespace
> in the cc file.

Or hmm, do they need to be here because of friendships?

[rmcilroy, 2016-12-12 09:50:39]

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
File src/compiler-dispatcher/compiler-dispatcher-job.h (right):

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher-job.h:60: bool
IsProcessing(Handle<SharedFunctionInfo> shared) const;
On 2016/12/12 09:00:02, marja wrote:
> Naming nit: IsProcessing has a connotation that something is "in progress",
and
> it's not clear that IsProcessing is true after the compilation job has
finished
> too.
> 
> IsAssociatedWith?

Or how about just a getter which returns the SFI and do the equality check in
the caller?

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
File src/compiler-dispatcher/compiler-dispatcher.cc (right):

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:29: // enqueued, we might not be
able to enqueue it.
On 2016/12/12 09:00:02, marja wrote:
> Hmm, this is weird. So we cannot enqueue things because we're not able to
assign
> unique IDs for them?
> 
> So we need to store the jobs so that they're searchable by keys... but is
there
> anything preventing us from having multiple jobs with the same key, if we have
a
> data structure that supports that? E.g., do we always have the
> SharedFunctionInfo available that we can check for real equality (not just
> equality of the IDs) when we need to.
> 
> (I guess we need to check the real equality anyway, since the IDs are not
> unique, so if we later check stuff like "is this function in the queue", it's
> not enough to check based on IDs. So it should work.)

Also, does this mean we are only able to enqueue one function per script at a
time? This seems like a limitation we'd want to avoid if we want to be able to
compile in parallel. +1 to Marja's suggestion.

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:50: // TODO(jochen): Check if
there's an in-flight background task working on this
Could you DCHECK there is no background thread processing it until this TODO is
done?

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:68: // TODO(jochen): Check if
there's an in-flight background task working on this
Ditto (and below).

[marja, 2016-12-12 09:54:59]

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
File src/compiler-dispatcher/compiler-dispatcher.cc (right):

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:29: // enqueued, we might not be
able to enqueue it.
On 2016/12/12 09:50:39, rmcilroy wrote:
> On 2016/12/12 09:00:02, marja wrote:
> > Hmm, this is weird. So we cannot enqueue things because we're not able to
> assign
> > unique IDs for them?
> > 
> > So we need to store the jobs so that they're searchable by keys... but is
> there
> > anything preventing us from having multiple jobs with the same key, if we
have
> a
> > data structure that supports that? E.g., do we always have the
> > SharedFunctionInfo available that we can check for real equality (not just
> > equality of the IDs) when we need to.
> > 
> > (I guess we need to check the real equality anyway, since the IDs are not
> > unique, so if we later check stuff like "is this function in the queue",
it's
> > not enough to check based on IDs. So it should work.)
> 
> Also, does this mean we are only able to enqueue one function per script at a
> time? This seems like a limitation we'd want to avoid if we want to be able to
> compile in parallel. +1 to Marja's suggestion.

I don't think it means that, since the key is the (script id, function id) pair.

[rmcilroy, 2016-12-12 10:08:30]

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
File src/compiler-dispatcher/compiler-dispatcher.cc (right):

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:29: // enqueued, we might not be
able to enqueue it.
On 2016/12/12 09:54:59, marja wrote:
> On 2016/12/12 09:50:39, rmcilroy wrote:
> > On 2016/12/12 09:00:02, marja wrote:
> > > Hmm, this is weird. So we cannot enqueue things because we're not able to
> > assign
> > > unique IDs for them?
> > > 
> > > So we need to store the jobs so that they're searchable by keys... but is
> > there
> > > anything preventing us from having multiple jobs with the same key, if we
> have
> > a
> > > data structure that supports that? E.g., do we always have the
> > > SharedFunctionInfo available that we can check for real equality (not just
> > > equality of the IDs) when we need to.
> > > 
> > > (I guess we need to check the real equality anyway, since the IDs are not
> > > unique, so if we later check stuff like "is this function in the queue",
> it's
> > > not enough to check based on IDs. So it should work.)
> > 
> > Also, does this mean we are only able to enqueue one function per script at
a
> > time? This seems like a limitation we'd want to avoid if we want to be able
to
> > compile in parallel. +1 to Marja's suggestion.
> 
> I don't think it means that, since the key is the (script id, function id)
pair.

Ahh right enough, I was thinking the pair was for the insert.

[jochen (gone - plz use gerrit), 2016-12-12 10:51:13]

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

[commit-bot: I haz the power, 2016-12-12 10:51:16]

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), 2016-12-12 10:51:17]

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
File src/compiler-dispatcher/compiler-dispatcher-job.h (right):

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher-job.h:60: bool
IsProcessing(Handle<SharedFunctionInfo> shared) const;
On 2016/12/12 at 09:00:02, marja wrote:
> Naming nit: IsProcessing has a connotation that something is "in progress",
and it's not clear that IsProcessing is true after the compilation job has
finished too.
> 
> IsAssociatedWith?

done

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher-job.h:60: bool
IsProcessing(Handle<SharedFunctionInfo> shared) const;
On 2016/12/12 at 09:50:39, rmcilroy wrote:
> On 2016/12/12 09:00:02, marja wrote:
> > Naming nit: IsProcessing has a connotation that something is "in progress",
and
> > it's not clear that IsProcessing is true after the compilation job has
finished
> > too.
> > 
> > IsAssociatedWith?
> 
> Or how about just a getter which returns the SFI and do the equality check in
the caller?

that would require creating a new handle, as the member of the job is a global
handle, so we can't just return it

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
File src/compiler-dispatcher/compiler-dispatcher.cc (right):

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:29: // enqueued, we might not be
able to enqueue it.
On 2016/12/12 at 09:00:02, marja wrote:
> Hmm, this is weird. So we cannot enqueue things because we're not able to
assign unique IDs for them?
> 
> So we need to store the jobs so that they're searchable by keys... but is
there anything preventing us from having multiple jobs with the same key, if we
have a data structure that supports that? E.g., do we always have the
SharedFunctionInfo available that we can check for real equality (not just
equality of the IDs) when we need to.
> 
> (I guess we need to check the real equality anyway, since the IDs are not
unique, so if we later check stuff like "is this function in the queue", it's
not enough to check based on IDs. So it should work.)

done

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:46: std::pair<int, int>
key(Script::cast(function->script())->id(),
On 2016/12/12 at 09:00:02, marja wrote:
> How about having a job-finding helper function - it's confusing that the
ID-based check is not enough? Also, this code does find twice (first in
IsEnqueued and then here), that's unnecessary.
> 
> (In the version I suggest a helper function would also make it clearer.)

done

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:50: // TODO(jochen): Check if
there's an in-flight background task working on this
On 2016/12/12 at 09:50:39, rmcilroy wrote:
> Could you DCHECK there is no background thread processing it until this TODO
is done?

there's currently no way to check whether a job is running on a background
thread

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
File src/compiler-dispatcher/compiler-dispatcher.h (right):

https://codereview.chromium.org/2558293004/diff/20001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.h:50: static bool
DoNextStepOnMainThread(CompilerDispatcherJob* job);
On 2016/12/12 at 09:02:08, marja wrote:
> On 2016/12/12 09:00:02, marja wrote:
> > Afaics these don't need to be in the header, they can be in an unnamed
namespace
> > in the cc file.
> 
> Or hmm, do they need to be here because of friendships?

yeah, I was first thinking about adding unit tests, but then the test looked
pretty much like a copy of the actual method (which makes it a bad test imo)

[jochen (gone - plz use gerrit), 2016-12-12 10:54:59]

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

[commit-bot: I haz the power, 2016-12-12 10:55:07]

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), 2016-12-12 10:55:10]

backed out the runtime-compiler.cc change as it's not needed yet

[commit-bot: I haz the power, 2016-12-12 11:21:22]

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

[commit-bot: I haz the power, 2016-12-12 11:21:23]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2016-12-12 12:17:48]

Description was changed from

==========
Add a basic compiler dispatcher

It doesn't schedule steps yet, but there are tests, and it's hooked up
to the CompileLazy builtin.

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

to

==========
Add a basic compiler dispatcher

It doesn't schedule steps yet, but there are tests.

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

[marja, 2016-12-12 12:25:01]

lgtm %

https://codereview.chromium.org/2558293004/diff/60001/src/compiler-dispatcher...
File src/compiler-dispatcher/compiler-dispatcher.cc (right):

https://codereview.chromium.org/2558293004/diff/60001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:64: bool
CompilerDispatcher::Enqueue(Handle<SharedFunctionInfo> function) {
Now the return value is not meaningful any more.

https://codereview.chromium.org/2558293004/diff/60001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:79: bool
CompilerDispatcher::FinishNow(Handle<SharedFunctionInfo> function) {
How would we use the return value? false can mean either that there was no job
in the queue (maybe a background task finished it just before the call) or that
the job failed. What would the return value used for?

There are 3 possible outcomes:
1. was not in the queue
2. was in the queue, we executed it and the compilation passed
3. was in the queue, we executed it and the compilation failed

and the return value can differentiate only between 2, so, depending on the
usage the grouping might be either (1, 2 & 3) (would have been my intuition) or
(1 & 3, 2) (like it is now).

[jochen (gone - plz use gerrit), 2016-12-12 12:33:56]

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

[commit-bot: I haz the power, 2016-12-12 12:34:03]

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), 2016-12-12 12:34:38]

https://codereview.chromium.org/2558293004/diff/60001/src/compiler-dispatcher...
File src/compiler-dispatcher/compiler-dispatcher.cc (right):

https://codereview.chromium.org/2558293004/diff/60001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:64: bool
CompilerDispatcher::Enqueue(Handle<SharedFunctionInfo> function) {
On 2016/12/12 at 12:25:01, marja wrote:
> Now the return value is not meaningful any more.

I added a meaningful "return false" code path

https://codereview.chromium.org/2558293004/diff/60001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:79: bool
CompilerDispatcher::FinishNow(Handle<SharedFunctionInfo> function) {
On 2016/12/12 at 12:25:01, marja wrote:
> How would we use the return value? false can mean either that there was no job
in the queue (maybe a background task finished it just before the call) or that
the job failed. What would the return value used for?
> 
> There are 3 possible outcomes:
> 1. was not in the queue
> 2. was in the queue, we executed it and the compilation passed
> 3. was in the queue, we executed it and the compilation failed
> 
> and the return value can differentiate only between 2, so, depending on the
usage the grouping might be either (1, 2 & 3) (would have been my intuition) or
(1 & 3, 2) (like it is now).

I added "UNREACHABLE()" to the "not in queue" case, as the caller should make
sure to first check whether the job is enqueued at all

[marja, 2016-12-12 12:41:36]

still lgtm

[commit-bot: I haz the power, 2016-12-12 13:01:33]

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

[commit-bot: I haz the power, 2016-12-12 13:01:34]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-12-12 14:00:20]

LGTM, just nits.

https://codereview.chromium.org/2558293004/diff/80001/src/compiler-dispatcher...
File src/compiler-dispatcher/compiler-dispatcher.cc (right):

https://codereview.chromium.org/2558293004/diff/80001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:50: bool
IsDone(CompilerDispatcherJob* job) {
nit - IsFinished or something to avoid confusion with CompileJobStatus::kDone?

https://codereview.chromium.org/2558293004/diff/80001/src/compiler-dispatcher...
src/compiler-dispatcher/compiler-dispatcher.cc:108: return;
nit - how about just do CHECK(job != Jobs_.end())?

[rmcilroy, 2016-12-12 14:00:23]

LGTM, just nits.

[jochen (gone - plz use gerrit), 2016-12-12 15:07:15]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2016-12-12 15:07:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2558293004/#ps100001
(title: "updates")

[commit-bot: I haz the power, 2016-12-12 15:07:20]

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, 2016-12-12 15:35:42]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481555235745160,
"parent_rev": "869bd90be7acfbd11268729af830e8876c74efdd", "commit_rev":
"2b13cf57eefa56ac57b1413513ebf4ca6acd19fa"}

[commit-bot: I haz the power, 2016-12-12 15:35:46]

Description was changed from

==========
Add a basic compiler dispatcher

It doesn't schedule steps yet, but there are tests.

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

to

==========
Add a basic compiler dispatcher

It doesn't schedule steps yet, but there are tests.

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

Review-Url: https://codereview.chromium.org/2558293004
==========

[commit-bot: I haz the power, 2016-12-12 15:35:47]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-12 15:36:33]

Description was changed from

==========
Add a basic compiler dispatcher

It doesn't schedule steps yet, but there are tests.

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

Review-Url: https://codereview.chromium.org/2558293004
==========

to

==========
Add a basic compiler dispatcher

It doesn't schedule steps yet, but there are tests.

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

Committed: https://crrev.com/1bdf908db0512f65eb10c6b167f2d108cc06b673
Cr-Commit-Position: refs/heads/master@{#41650}
==========

[commit-bot: I haz the power, 2016-12-12 15:36:34]

Patchset 6 (id:??) landed as
https://crrev.com/1bdf908db0512f65eb10c6b167f2d108cc06b673
Cr-Commit-Position: refs/heads/master@{#41650}