[Compiler] Avoid blocking on inner function parallel compilation.

[Compiler] Avoid blocking on inner function parallel compilation.

Don't block on inner function compilation before competing outer function
compilation. Instead wait for the compilation to complete when the function
is called.

BUG=v8:5203
Review-Url: https://codereview.chromium.org/2686673002
Cr-Commit-Position: refs/heads/master@{#43116}
Committed: https://chromium.googlesource.com/v8/v8/+/53c11006e5a57c248d858cfcaa488493a31aabe4

[rmcilroy, 2017-02-07 20:59:45]

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

[commit-bot: I haz the power, 2017-02-07 20:59:51]

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

[rmcilroy, 2017-02-07 21:26:20]

rmcilroy@chromium.org changed reviewers:
+ jochen@chromium.org, marja@chromium.org, mstarzinger@chromium.org

[rmcilroy, 2017-02-07 21:26:21]

Michi - PTAL at compiler.
Marja / Jochen PTAL at compiler-dispatcher.

[commit-bot: I haz the power, 2017-02-07 21:30:53]

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

[commit-bot: I haz the power, 2017-02-07 21:30:53]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2017-02-07 22:34:53]

Patchset #1 (id:1) has been deleted

[jochen (gone - plz use gerrit), 2017-02-08 08:56:39]

is it possible to add tests for this?

[rmcilroy, 2017-02-08 23:18:27]

On 2017/02/08 08:56:39, jochen wrote:
> is it possible to add tests for this?

There is a test in the previous CL (https://codereview.chromium.org/2650883002/)
for ensuring we can compile after the HandleScope / Zones / ParseInfo's created
during parsing have gone out-of-scope. I've added a test in this CL that
CompileLazy will finish a compiler-dispatcher job if it exists rather than
re-compiling. PTAL, thanks.

[rmcilroy, 2017-02-08 23:32:26]

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

[commit-bot: I haz the power, 2017-02-08 23:32:34]

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-02-08 23:42:18]

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

[commit-bot: I haz the power, 2017-02-08 23:42:19]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[marja, 2017-02-09 09:09:48]

lgtm, but jochen@ knows this code better than me...

https://codereview.chromium.org/2686673002/diff/60001/test/unittests/compiler...
File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right):

https://codereview.chromium.org/2686673002/diff/60001/test/unittests/compiler...
test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:913: "}
g();";
Is this g() now unnecessary? It's a bit confusing...

[rmcilroy, 2017-02-09 10:37:57]

https://codereview.chromium.org/2686673002/diff/60001/test/unittests/compiler...
File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right):

https://codereview.chromium.org/2686673002/diff/60001/test/unittests/compiler...
test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:913: "}
g();";
On 2017/02/09 09:09:48, marja wrote:
> Is this g() now unnecessary? It's a bit confusing...

This is still needed (it returns f16 as the return value of the statement, which
is used to initialize Handle<JSFunction> f. f16 isn't actually called until the
"g()()" below.

[jochen (gone - plz use gerrit), 2017-02-09 13:15:18]

lgtm

[Michael Starzinger, 2017-02-09 13:58:56]

LGTM on the compiler pipeline plumbing.

[marja, 2017-02-09 16:22:01]

https://codereview.chromium.org/2686673002/diff/60001/test/unittests/compiler...
File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right):

https://codereview.chromium.org/2686673002/diff/60001/test/unittests/compiler...
test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:913: "}
g();";
On 2017/02/09 10:37:57, rmcilroy wrote:
> On 2017/02/09 09:09:48, marja wrote:
> > Is this g() now unnecessary? It's a bit confusing...
> 
> This is still needed (it returns f16 as the return value of the statement,
which
> is used to initialize Handle<JSFunction> f. f16 isn't actually called until
the
> "g()()" below.

Acknowledged.

https://codereview.chromium.org/2686673002/diff/60001/test/unittests/compiler...
test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:927: }
Would it be interesting to add a test like this:

function lazy() { lazy2(); }
function lazy2() { ... }

enqueue(lazy);
enqueue(lazy2); << need some trickery to get handles to both funcs
RunJS(lazy);
assert_false(isenqueued(lazy2));

[rmcilroy, 2017-02-10 11:21:16]

https://codereview.chromium.org/2686673002/diff/60001/test/unittests/compiler...
File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right):

https://codereview.chromium.org/2686673002/diff/60001/test/unittests/compiler...
test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:927: }
On 2017/02/09 16:22:01, marja wrote:
> Would it be interesting to add a test like this:
> 
> function lazy() { lazy2(); }
> function lazy2() { ... }
> 
> enqueue(lazy);
> enqueue(lazy2); << need some trickery to get handles to both funcs
> RunJS(lazy);
> assert_false(isenqueued(lazy2));
> 

Sure, added this test. It ended up being slightly more complicated (I needed to
make sure lazy1 and lazy2 captured a variable from an outer scope due to the:
DCHECK(!shared_->outer_scope_info()->IsTheHole(isolate_));
in compiler-dispatcher-job.cc constructor. (see comment on new patch set).

https://codereview.chromium.org/2686673002/diff/80001/test/unittests/compiler...
File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right):

https://codereview.chromium.org/2686673002/diff/80001/test/unittests/compiler...
test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:936: "  var y
= 42; "
Jochen, as discussed offline, I've had to add this extra local to be captured by
lazy2 (and the same on lazy1) in order to avoid the
"DCHECK(!shared_->outer_scope_info()->IsTheHole(isolate_));". Is this DCHECK
necessary?

[rmcilroy, 2017-02-10 16:15:37]

https://codereview.chromium.org/2686673002/diff/80001/test/unittests/compiler...
File test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc (right):

https://codereview.chromium.org/2686673002/diff/80001/test/unittests/compiler...
test/unittests/compiler-dispatcher/compiler-dispatcher-unittest.cc:936: "  var y
= 42; "
On 2017/02/10 11:21:16, rmcilroy wrote:
> Jochen, as discussed offline, I've had to add this extra local to be captured
by
> lazy2 (and the same on lazy1) in order to avoid the
> "DCHECK(!shared_->outer_scope_info()->IsTheHole(isolate_));". Is this DCHECK
> necessary?

As discussed offline, changed the DCHECK to !shared->is_toplevel() and
simplified the test.

[rmcilroy, 2017-02-10 16:15:49]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2017-02-10 16:15:50]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mstarzinger@chromium.org, marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2686673002/#ps100001
(title: "Change dcheck and simplify test")

[commit-bot: I haz the power, 2017-02-10 16:16:00]

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-02-10 16:19:30]

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

[commit-bot: I haz the power, 2017-02-10 16:19:31]

Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)

[rmcilroy, 2017-02-10 16:42:38]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2017-02-10 16:42:38]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mstarzinger@chromium.org, marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2686673002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-10 16:42:47]

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-02-10 17:13:32]

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

[commit-bot: I haz the power, 2017-02-10 17:13:33]

Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/20836)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)

[rmcilroy, 2017-02-10 17:52:13]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2017-02-10 17:52:14]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mstarzinger@chromium.org, marja@chromium.org
Link to the patchset: https://codereview.chromium.org/2686673002/#ps140001
(title: "Fix outer_scope_info handling")

[commit-bot: I haz the power, 2017-02-10 17:52:19]

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-02-10 18:46:32]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1486749133934340,
"parent_rev": "2bfd8a7cb7c9929e667fddc2776e1fc628d2b73b", "commit_rev":
"53c11006e5a57c248d858cfcaa488493a31aabe4"}

[commit-bot: I haz the power, 2017-02-10 18:46:36]

Description was changed from

==========
[Compiler] Avoid blocking on inner function parallel compilation.

Don't block on inner function compilation before competing outer function
compilation. Instead wait for the compilation to complete when the function
is called.

BUG=v8:5203
==========

to

==========
[Compiler] Avoid blocking on inner function parallel compilation.

Don't block on inner function compilation before competing outer function
compilation. Instead wait for the compilation to complete when the function
is called.

BUG=v8:5203

Review-Url: https://codereview.chromium.org/2686673002
Cr-Commit-Position: refs/heads/master@{#43116}
Committed:
https://chromium.googlesource.com/v8/v8/+/53c11006e5a57c248d858cfcaa488493a31...
==========

[commit-bot: I haz the power, 2017-02-10 18:46:37]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/v8/v8/+/53c11006e5a57c248d858cfcaa488493a31...