[Interpreter] Make ignition compiler eagerly.

[Interpreter] Make ignition compiler eagerly.

Makes --ignition cause eager compilation if we aren't building the startup
snapshot.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/838cea4e4ee0e473703e1a1db2ead95dde039684
Cr-Commit-Position: refs/heads/master@{#35066}

[rmcilroy, 2016-03-17 14:38:11]

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

[rmcilroy, 2016-03-17 14:38:12]

Michi PTAL. I've kept this logic similar to how FLAG_serialize_eager was added,
but I'm wondering whether it would make sense to split some of this logic out to
a common helper routine - let me know what you think?

[Michael Starzinger, 2016-03-17 18:58:51]

LGTM. Let's not yet factor it out. I started massaging compiler.cc today and
will tackle it once I get to the parsing part. I'll keep you posted.

https://codereview.chromium.org/1811553003/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1811553003/diff/1/src/compiler.cc#newcode1416
src/compiler.cc:1416: // Consider compiling eagerly when compiling bytecode for
Ignition.
nit: s/compiling/parsing/

[rmcilroy, 2016-03-21 15:21:55]

https://codereview.chromium.org/1811553003/diff/1/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1811553003/diff/1/src/compiler.cc#newcode1416
src/compiler.cc:1416: // Consider compiling eagerly when compiling bytecode for
Ignition.
On 2016/03/17 18:58:51, Michael Starzinger wrote:
> nit: s/compiling/parsing/

Done.

[rmcilroy, 2016-03-22 15:35:47]

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

[commit-bot: I haz the power, 2016-03-22 15:35:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1811553003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1811553003/40001

[commit-bot: I haz the power, 2016-03-22 15:42:25]

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

[commit-bot: I haz the power, 2016-03-22 15:42:26]

Dry run: Try jobs failed on following builders:
  v8_linux_arm64_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel/builds/1...)

[rmcilroy, 2016-03-22 15:43:24]

rmcilroy@chromium.org changed reviewers:
+ mythria@chromium.org, yangguo@chromium.org

[rmcilroy, 2016-03-22 15:43:25]

Michi - I had to fix a bunch of tests and add a couple more skips for various
failures (although I got to remove a couple too :)). Could you take another look
please?

Yang / Mythri - FYI - please see comments with your names below:

https://codereview.chromium.org/1811553003/diff/40001/src/background-parsing-...
File src/background-parsing-task.cc (right):

https://codereview.chromium.org/1811553003/diff/40001/src/background-parsing-...
src/background-parsing-task.cc:36: info->set_allow_lazy_parsing(!FLAG_ignition);
Yang - I was getting crashes for certain tests before changing this due to the
background parser not parsing eagerly, but Ignition trying to compile eagerly
(on functions with no parse data). I'm not sure if you would ever see the same
with your changes to eagerly compile functions when building a code snapshot -
would V8 ever use the background parser and then try and compile eagerly with
your change?

https://codereview.chromium.org/1811553003/diff/40001/test/cctest/cctest.status
File test/cctest/cctest.status (right):

https://codereview.chromium.org/1811553003/diff/40001/test/cctest/cctest.stat...
test/cctest/cctest.status:505: 'test-cpu-profiler/TickLinesOptimized': [FAIL],
Michi - looks like this test is back :).

https://codereview.chromium.org/1811553003/diff/40001/test/cctest/cctest.stat...
test/cctest/cctest.status:510:
'test-func-name-inference/ReturnAnonymousFunction': [FAIL],
Mythri - could you have a look at this test when you get a second, it also fails
with --no-lazy (without ignition) so it might be an old existing bug somewhere.

https://codereview.chromium.org/1811553003/diff/40001/test/cctest/cctest.stat...
test/cctest/cctest.status:534: 'test-profile-generator/BailoutReason': [FAIL],
Michi - I guess this is something which will be fixed by your compiler.cc
refactoring?

https://codereview.chromium.org/1811553003/diff/40001/test/cctest/cctest.stat...
test/cctest/cctest.status:539: 'test-api/TurboAsmDisablesNeuter': [FAIL],
Michi - ditto, hopefully the lazy and eager pipelines will get the same checks
for turbo-asm going forward?

https://codereview.chromium.org/1811553003/diff/40001/test/mjsunit/mjsunit.st...
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/1811553003/diff/40001/test/mjsunit/mjsunit.st...
test/mjsunit/mjsunit.status:806: 'harmony/regress/regress-4658': [FAIL],
Mythri - when you get a second could you have a look at this? It might be a real
bug in Ignition or the part of the parser which tells us how many context
registers to use (which I think is only used by Ignition).

https://codereview.chromium.org/1811553003/diff/40001/test/mjsunit/mjsunit.st...
test/mjsunit/mjsunit.status:810: # f(), so we can't catch the exception in the
try/catch.
Michi - Any idea if we could modify this test to allow it to pass with eager
compiling?

[rmcilroy, 2016-03-22 15:45:57]

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

[commit-bot: I haz the power, 2016-03-22 15:46:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1811553003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1811553003/60001

[commit-bot: I haz the power, 2016-03-22 16:03:42]

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

[commit-bot: I haz the power, 2016-03-22 16:03:43]

Dry run: Try jobs failed on following builders:
  v8_win_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/4794)
  v8_win_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)

[Yang, 2016-03-23 14:29:12]

LGTM.

https://codereview.chromium.org/1811553003/diff/40001/src/background-parsing-...
File src/background-parsing-task.cc (right):

https://codereview.chromium.org/1811553003/diff/40001/src/background-parsing-...
src/background-parsing-task.cc:36: info->set_allow_lazy_parsing(!FLAG_ignition);
On 2016/03/22 15:43:25, rmcilroy wrote:
> Yang - I was getting crashes for certain tests before changing this due to the
> background parser not parsing eagerly, but Ignition trying to compile eagerly
> (on functions with no parse data). I'm not sure if you would ever see the same
> with your changes to eagerly compile functions when building a code snapshot -
> would V8 ever use the background parser and then try and compile eagerly with
> your change?

The code snapshot never runs into the issue with the background parser because
we only ever serialize on second compile. Which loads the source from cache, and
doesn't stream it from network.

Not too familiar with this part of the code :/

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

https://codereview.chromium.org/1811553003/diff/60001/src/compiler.cc#newcode...
src/compiler.cc:1280: parse_allow_lazy &= !(FLAG_ignition &&
!isolate->serializer_enabled());
Not sure whether we need the serializer_enabled part. I don't think there is a
difference since we throw away bytecode arrays on serialization anyways. But
maybe we will change that in the future and include bytecode  arrays in the
snapshot.

[rmcilroy, 2016-03-24 11:30:45]

Addressed comments and added a new flag as discussed Michi, PTAL.

https://codereview.chromium.org/1811553003/diff/40001/src/background-parsing-...
File src/background-parsing-task.cc (right):

https://codereview.chromium.org/1811553003/diff/40001/src/background-parsing-...
src/background-parsing-task.cc:36: info->set_allow_lazy_parsing(!FLAG_ignition);
On 2016/03/23 14:29:11, Yang wrote:
> On 2016/03/22 15:43:25, rmcilroy wrote:
> > Yang - I was getting crashes for certain tests before changing this due to
the
> > background parser not parsing eagerly, but Ignition trying to compile
eagerly
> > (on functions with no parse data). I'm not sure if you would ever see the
same
> > with your changes to eagerly compile functions when building a code snapshot
-
> > would V8 ever use the background parser and then try and compile eagerly
with
> > your change?
> 
> The code snapshot never runs into the issue with the background parser because
> we only ever serialize on second compile. Which loads the source from cache,
and
> doesn't stream it from network.
> 
> Not too familiar with this part of the code :/

Yeah makes sense, just checking.

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

https://codereview.chromium.org/1811553003/diff/60001/src/compiler.cc#newcode...
src/compiler.cc:1280: parse_allow_lazy &= !(FLAG_ignition &&
!isolate->serializer_enabled());
On 2016/03/23 14:29:11, Yang wrote:
> Not sure whether we need the serializer_enabled part. I don't think there is a
> difference since we throw away bytecode arrays on serialization anyways. But
> maybe we will change that in the future and include bytecode  arrays in the
> snapshot.

Yeah, the issue here is that if we eagerly compile the snapshot, we eagerly
compile the builtin functions before they are assigned builtin-ids, which means
we can't blacklist the generator related builtins (which don't compile in
ignition - see UseIgnition()).

[Yang, 2016-03-24 11:37:26]

On 2016/03/24 11:30:45, rmcilroy wrote:
> Addressed comments and added a new flag as discussed Michi, PTAL.
> 
>
https://codereview.chromium.org/1811553003/diff/40001/src/background-parsing-...
> File src/background-parsing-task.cc (right):
> 
>
https://codereview.chromium.org/1811553003/diff/40001/src/background-parsing-...
> src/background-parsing-task.cc:36:
info->set_allow_lazy_parsing(!FLAG_ignition);
> On 2016/03/23 14:29:11, Yang wrote:
> > On 2016/03/22 15:43:25, rmcilroy wrote:
> > > Yang - I was getting crashes for certain tests before changing this due to
> the
> > > background parser not parsing eagerly, but Ignition trying to compile
> eagerly
> > > (on functions with no parse data). I'm not sure if you would ever see the
> same
> > > with your changes to eagerly compile functions when building a code
snapshot
> -
> > > would V8 ever use the background parser and then try and compile eagerly
> with
> > > your change?
> > 
> > The code snapshot never runs into the issue with the background parser
because
> > we only ever serialize on second compile. Which loads the source from cache,
> and
> > doesn't stream it from network.
> > 
> > Not too familiar with this part of the code :/
> 
> Yeah makes sense, just checking.
> 
> https://codereview.chromium.org/1811553003/diff/60001/src/compiler.cc
> File src/compiler.cc (right):
> 
>
https://codereview.chromium.org/1811553003/diff/60001/src/compiler.cc#newcode...
> src/compiler.cc:1280: parse_allow_lazy &= !(FLAG_ignition &&
> !isolate->serializer_enabled());
> On 2016/03/23 14:29:11, Yang wrote:
> > Not sure whether we need the serializer_enabled part. I don't think there is
a
> > difference since we throw away bytecode arrays on serialization anyways. But
> > maybe we will change that in the future and include bytecode  arrays in the
> > snapshot.
> 
> Yeah, the issue here is that if we eagerly compile the snapshot, we eagerly
> compile the builtin functions before they are assigned builtin-ids, which
means
> we can't blacklist the generator related builtins (which don't compile in
> ignition - see UseIgnition()).

I see. lgtm.

[rmcilroy, 2016-03-24 12:40:50]

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

[commit-bot: I haz the power, 2016-03-24 12:40:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1811553003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1811553003/80001

[commit-bot: I haz the power, 2016-03-24 13:04:22]

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

[commit-bot: I haz the power, 2016-03-24 13:04:24]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-03-24 13:27:14]

Yep LGTM again. Thanks for introducing the new flag!

https://codereview.chromium.org/1811553003/diff/80001/test/cctest/test-heap-p...
File test/cctest/test-heap-profiler.cc (right):

https://codereview.chromium.org/1811553003/diff/80001/test/cctest/test-heap-p...
test/cctest/test-heap-profiler.cc:370: if (i::FLAG_lazy && !i::FLAG_ignition) {
nit: Can we also check for "FLAG_ignition_eager" here? Or am I missing something
in this case?

[rmcilroy, 2016-03-24 13:48:26]

https://codereview.chromium.org/1811553003/diff/80001/test/cctest/test-heap-p...
File test/cctest/test-heap-profiler.cc (right):

https://codereview.chromium.org/1811553003/diff/80001/test/cctest/test-heap-p...
test/cctest/test-heap-profiler.cc:370: if (i::FLAG_lazy && !i::FLAG_ignition) {
On 2016/03/24 13:27:14, Michael Starzinger wrote:
> nit: Can we also check for "FLAG_ignition_eager" here? Or am I missing
something
> in this case?

Missed this, good catch, thanks. Done

[rmcilroy, 2016-03-24 13:55:18]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2016-03-24 13:55:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/1811553003/#ps100001
(title: "Fix test which was missed")

[commit-bot: I haz the power, 2016-03-24 13:55:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1811553003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1811553003/100001

[commit-bot: I haz the power, 2016-03-24 13:56:37]

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

[commit-bot: I haz the power, 2016-03-24 13:56:38]

Try jobs failed on following builders:
  v8_linux64_asan_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel/builds/...)
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...)
  v8_linux_gcc_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
  v8_linux_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/3432)
  v8_mac_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/17556)
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/12858)

[rmcilroy, 2016-03-24 15:25:18]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2016-03-24 15:25:18]

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

[commit-bot: I haz the power, 2016-03-24 15:25:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1811553003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1811553003/120001

[commit-bot: I haz the power, 2016-03-24 17:13:31]

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

[commit-bot: I haz the power, 2016-03-24 17:13:34]

Failed to apply patch for
test/cctest/compiler/test-run-bytecode-graph-builder.cc:
While running git apply --index -3 -p1;
  error: patch failed:
test/cctest/compiler/test-run-bytecode-graph-builder.cc:79
  Falling back to three-way merge...
  Applied patch to 'test/cctest/compiler/test-run-bytecode-graph-builder.cc'
with conflicts.
  U test/cctest/compiler/test-run-bytecode-graph-builder.cc

Patch:       test/cctest/compiler/test-run-bytecode-graph-builder.cc
Index: test/cctest/compiler/test-run-bytecode-graph-builder.cc
diff --git a/test/cctest/compiler/test-run-bytecode-graph-builder.cc
b/test/cctest/compiler/test-run-bytecode-graph-builder.cc
index
b63e55218359989752f9492cc400623e41d13645..62bf56ffda4a68cba338cf4ff027f8d4e3d8ace2
100644
--- a/test/cctest/compiler/test-run-bytecode-graph-builder.cc
+++ b/test/cctest/compiler/test-run-bytecode-graph-builder.cc
@@ -79,12 +79,6 @@ class BytecodeGraphTester {
     i::FLAG_always_opt = false;
     i::FLAG_allow_natives_syntax = true;
     i::FLAG_loop_assignment_analysis = false;
-    // Set ignition filter flag via SetFlagsFromString to avoid double-free
-    // (or potential leak with StrDup() based on ownership confusion).
-    ScopedVector<char> ignition_filter(64);
-    SNPrintF(ignition_filter, "--ignition-filter=%s", filter);
-    FlagList::SetFlagsFromString(ignition_filter.start(),
-                                 ignition_filter.length());
   }
   virtual ~BytecodeGraphTester() {}

[rmcilroy, 2016-03-24 18:14:46]

Patchset #7 (id:120001) has been deleted

[rmcilroy, 2016-03-24 18:14:55]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2016-03-24 18:14:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1811553003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1811553003/100001

[commit-bot: I haz the power, 2016-03-24 18:38:02]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-24 18:38:33]

Description was changed from

==========
[Interpreter] Make ignition compiler eagerly.

Makes --ignition cause eager compilation if we aren't building the startup
snapshot.

BUG=v8:4280
LOG=N
==========

to

==========
[Interpreter] Make ignition compiler eagerly.

Makes --ignition cause eager compilation if we aren't building the startup
snapshot.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/838cea4e4ee0e473703e1a1db2ead95dde039684
Cr-Commit-Position: refs/heads/master@{#35066}
==========

[commit-bot: I haz the power, 2016-03-24 18:38:34]

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