[compiler] Collect eager inner functions for compilation during renumbering.

[compiler] Collect eager inner functions for compilation during renumbering.

This CL modifies the ast-numbering phase to collect function literals which
should be compiled eagerly. This is then used to eagerly compile the inner
functions before compiling the outer function. This will be used to queue
compilation jobs on the CompilerDispatcher in a later CL.

This CL moves the compilation of eager inner functions out of the
GetSharedFunctionInfo function and instead compiles them explicitly. This
simplifies GetSharedFunctionInfo and also means there is no need to pass a
LazyCompilationMode to the function, so this concept has been removed.

BUG=v8:5203, v8:5215
Review-Url: https://codereview.chromium.org/2618553004
Cr-Commit-Position: refs/heads/master@{#42221}
Committed: https://chromium.googlesource.com/v8/v8/+/a3052cfe2209420afd7d9cb621103b384d23623e

[rmcilroy, 2017-01-05 15:41:21]

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

[commit-bot: I haz the power, 2017-01-05 15:41:26]

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

[rmcilroy, 2017-01-05 15:44:56]

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

[rmcilroy, 2017-01-05 15:44:57]

PTAL. This is the first step to sending the eager inner functions via the
compiler dispatcher. In the process inner functions no longer get compiled in
GetSharedFunctionInfo, so the LazyCompileMode is no longer required, therefore
this CL effectively reverts r41754.

[rmcilroy, 2017-01-05 15:46:45]

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

[commit-bot: I haz the power, 2017-01-05 15:46:57]

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-05 15:53:12]

some initial comments.

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

https://codereview.chromium.org/2618553004/diff/20001/src/compiler.cc#newcode...
src/compiler.cc:1656: return existing;
this might be wrong if we want debug code, but the existing SFI has only
baseline code

https://codereview.chromium.org/2618553004/diff/20001/src/compiler.cc#newcode...
src/compiler.cc:1661:
isolate->factory()->NewSharedFunctionInfoForLiteral(literal, script);
why don't you need to put the compile lazy builtin here?

[rmcilroy, 2017-01-05 16:02:42]

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

https://codereview.chromium.org/2618553004/diff/20001/src/compiler.cc#newcode...
src/compiler.cc:1656: return existing;
On 2017/01/05 15:53:12, jochen wrote:
> this might be wrong if we want debug code, but the existing SFI has only
> baseline code

The debug code logic didn't do anything any longer - there is a comment below in
the old code saying "When compiling for debug, we are not interested in having
debug break slots in inner functions" and the compilation logic was wrapped in a
if (!result->is_compiled()) so even though we fell through here, we were never
actually recompiling the inner function for debug code anyway.

https://codereview.chromium.org/2618553004/diff/20001/src/compiler.cc#newcode...
src/compiler.cc:1661:
isolate->factory()->NewSharedFunctionInfoForLiteral(literal, script);
On 2017/01/05 15:53:12, jochen wrote:
> why don't you need to put the compile lazy builtin here?

I was confused by this too, but NewSharedFunctionInfoForLiteral already does it
(the old code only added the lazy builtin to the ComilationInfo (not the new
SFI), presumably so that RecordFunctionCompilation "worked").

[commit-bot: I haz the power, 2017-01-05 16:11:34]

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

[commit-bot: I haz the power, 2017-01-05 16:11:35]

Dry run: This issue passed the CQ dry run.

[marja, 2017-01-05 19:25:23]

Do I understand this correctly: this doesn't change what gets compiled but just
shuffles the code around so that the compilation of eager functions is now done
based on the list, and not based on ast walking?

https://codereview.chromium.org/2618553004/diff/20001/src/ast/ast-numbering.h
File src/ast/ast-numbering.h (right):

https://codereview.chromium.org/2618553004/diff/20001/src/ast/ast-numbering.h...
src/ast/ast-numbering.h:23: ZoneVector<FunctionLiteral*>*
eager_inner_functions);
I was thinking about the ZoneVector usage here... the number of eager inner
functions can be big, and ZoneVectors are wasteful (every time it is expanded,
we cannot discard the old backing store).

Do you have some numbers based on which we could decide whether this choice of
data structure is fine or not? Can we initialize the ZoneVector intelligently so
that it wouldn't need to expand so much?

ThreadedList (sic) would be an alternative, but then you'd need to create a
separate class for holding the FunctionLiteral* and the next pointer. (Maybe
adding such a class in a templatized way would be a good idea.)

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

https://codereview.chromium.org/2618553004/diff/20001/src/compiler.cc#newcode505
src/compiler.cc:505: ParseInfo parse_info(&zone, script);
It's weird | surprising that we need to create a new ParseInfo here even if we
don't parse... it's because the renumbering needs one as input, right?

https://codereview.chromium.org/2618553004/diff/20001/test/unittests/compiler...
File test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc
(left):

https://codereview.chromium.org/2618553004/diff/20001/test/unittests/compiler...
test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc:350:
ASSERT_FALSE(e->shared()->HasBaselineCode());
Hmm, I don't understand why this test is removed.. is something it asserts now
false?

[rmcilroy, 2017-01-06 17:08:14]

> Do I understand this correctly: this doesn't change what gets compiled but
just
> shuffles the code around so that the compilation of eager functions is now
done
> based on the list, and not based on ast walking?

Yes exactly. The same set of functions should get eagerly compiled, however they
are now compiled before compiling the outer function, instead of in the middle
of compiling the outer function, in order to pave the way for enqueuing them on
the compiler dispatcher.

https://codereview.chromium.org/2618553004/diff/20001/src/ast/ast-numbering.h
File src/ast/ast-numbering.h (right):

https://codereview.chromium.org/2618553004/diff/20001/src/ast/ast-numbering.h...
src/ast/ast-numbering.h:23: ZoneVector<FunctionLiteral*>*
eager_inner_functions);
On 2017/01/05 19:25:22, marja wrote:
> I was thinking about the ZoneVector usage here... the number of eager inner
> functions can be big, and ZoneVectors are wasteful (every time it is expanded,
> we cannot discard the old backing store).
> 
> Do you have some numbers based on which we could decide whether this choice of
> data structure is fine or not? Can we initialize the ZoneVector intelligently
so
> that it wouldn't need to expand so much?
> 
> ThreadedList (sic) would be an alternative, but then you'd need to create a
> separate class for holding the FunctionLiteral* and the next pointer. (Maybe
> adding such a class in a templatized way would be a good idea.)

Yes good point, I'm not sure we have a good default which we could intelligently
initialize ZoneVector with. I'll have a look at ThreadedList instead.

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

https://codereview.chromium.org/2618553004/diff/20001/src/compiler.cc#newcode505
src/compiler.cc:505: ParseInfo parse_info(&zone, script);
On 2017/01/05 19:25:22, marja wrote:
> It's weird | surprising that we need to create a new ParseInfo here even if we
> don't parse... it's because the renumbering needs one as input, right?

Yes I agree this is a bit weird (it is a copy of the code which used to be in
GetSharedFunctionInfo and did the same thing). Yes it's because Renumber needs a
parse_info, and also because the compiler also gets some stuff from parse_info
indirectly via accessors on compile_info (e.g., the literal, script, scope,
shared_info, etc.). 

It would be really nice to disentangle parse_info and compilation_info so that
they don't refer to each other then we could maybe get away without this.

https://codereview.chromium.org/2618553004/diff/20001/test/unittests/compiler...
File test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc
(left):

https://codereview.chromium.org/2618553004/diff/20001/test/unittests/compiler...
test/unittests/compiler-dispatcher/compiler-dispatcher-job-unittest.cc:350:
ASSERT_FALSE(e->shared()->HasBaselineCode());
On 2017/01/05 19:25:22, marja wrote:
> Hmm, I don't understand why this test is removed.. is something it asserts now
> false?

This was part of the revert of the LazyCompileMode cl which introduced this
test, but you are right, the constraints are still the same and this test should
still pass, so adding it back.

[rmcilroy, 2017-01-09 16:50:11]

Comments addressed, PTAL.

https://codereview.chromium.org/2618553004/diff/20001/src/ast/ast-numbering.h
File src/ast/ast-numbering.h (right):

https://codereview.chromium.org/2618553004/diff/20001/src/ast/ast-numbering.h...
src/ast/ast-numbering.h:23: ZoneVector<FunctionLiteral*>*
eager_inner_functions);
On 2017/01/06 17:08:14, rmcilroy wrote:
> On 2017/01/05 19:25:22, marja wrote:
> > I was thinking about the ZoneVector usage here... the number of eager inner
> > functions can be big, and ZoneVectors are wasteful (every time it is
expanded,
> > we cannot discard the old backing store).
> > 
> > Do you have some numbers based on which we could decide whether this choice
of
> > data structure is fine or not? Can we initialize the ZoneVector
intelligently
> so
> > that it wouldn't need to expand so much?
> > 
> > ThreadedList (sic) would be an alternative, but then you'd need to create a
> > separate class for holding the FunctionLiteral* and the next pointer. (Maybe
> > adding such a class in a templatized way would be a good idea.)
> 
> Yes good point, I'm not sure we have a good default which we could
intelligently
> initialize ZoneVector with. I'll have a look at ThreadedList instead.

Done.

[rmcilroy, 2017-01-09 16:50:29]

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

[commit-bot: I haz the power, 2017-01-09 16:50:31]

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-09 16:51:40]

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

[commit-bot: I haz the power, 2017-01-09 16:51:42]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  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_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/15201)

[jochen (gone - plz use gerrit), 2017-01-10 09:57:39]

lgtm

https://codereview.chromium.org/2618553004/diff/50001/src/zone/zone-containers.h
File src/zone/zone-containers.h (right):

https://codereview.chromium.org/2618553004/diff/50001/src/zone/zone-container...
src/zone/zone-containers.h:155: };
disallow copy/assign?

[marja, 2017-01-10 10:10:43]

I was thinking about the lazy compilation flag; I'll probably need to
re-introduce a flag which says "compile everything eagerly" (because I need to
add an API which compiles and generates as complete code cache as possible). But
the current flag doesn't seem super useful for that purpose anyway, so, I'll
just re-initiate the one I need later.

LGTM %

https://codereview.chromium.org/2618553004/diff/50001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (left):

https://codereview.chromium.org/2618553004/diff/50001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:49:
DISALLOW_COPY_AND_ASSIGN(FullCodegenCompilationJob);
You probably didn't mean to delete the DISALLOW_...?

https://codereview.chromium.org/2618553004/diff/50001/src/zone/zone-containers.h
File src/zone/zone-containers.h (right):

https://codereview.chromium.org/2618553004/diff/50001/src/zone/zone-container...
src/zone/zone-containers.h:16: #include "src/utils.h"
Why?

[Michael Starzinger, 2017-01-10 12:26:10]

The compiler changes looking good, just one comment about the ThreadedList
helper class.

https://codereview.chromium.org/2618553004/diff/50001/src/zone/zone-containers.h
File src/zone/zone-containers.h (right):

https://codereview.chromium.org/2618553004/diff/50001/src/zone/zone-container...
src/zone/zone-containers.h:145: class ThreadedListZoneEntry final : public
ZoneObject {
This file is specifically meant to contain stdlib containers living on the zone
(and specifically don't subclass ZoneObject). Could we move this helper class
into the utils.h file instead, close to our hand-woven ThreadedList class?

[rmcilroy, 2017-01-10 23:34:16]

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

[commit-bot: I haz the power, 2017-01-10 23:34:25]

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-10 23:54:35]

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

[commit-bot: I haz the power, 2017-01-10 23:54:36]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[rmcilroy, 2017-01-11 00:34:39]

Patchset #5 (id:70001) has been deleted

[rmcilroy, 2017-01-11 00:34:49]

Patchset #5 (id:90001) has been deleted

[rmcilroy, 2017-01-11 00:34:54]

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

[commit-bot: I haz the power, 2017-01-11 00:34:58]

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-11 01:03:43]

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

[commit-bot: I haz the power, 2017-01-11 01:03:44]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2017-01-11 10:20:11]

I've reworked this to remove the eager_literals from ParseInfo and pass it
directly to Renumbering. This is because I've changed the followup CL so that
the CompilerDispatcher creates and ones it's own ParseInfo. PTAL, thanks.

> I was thinking about the lazy compilation flag; I'll probably need to
> re-introduce a flag which says "compile everything eagerly" (because I need to
> add an API which compiles and generates as complete code cache as possible).
But
> the current flag doesn't seem super useful for that purpose anyway, so, I'll
> just re-initiate the one I need later.

I'm not sure what flag you mean here. --no-lazy should compile everything
eagerly still AFAIK, although maybe you are wanting something which does this at
runtime. I think the will_serialize flag should do this, and I don't think this
CL will impact that.

https://codereview.chromium.org/2618553004/diff/50001/src/full-codegen/full-c...
File src/full-codegen/full-codegen.cc (left):

https://codereview.chromium.org/2618553004/diff/50001/src/full-codegen/full-c...
src/full-codegen/full-codegen.cc:49:
DISALLOW_COPY_AND_ASSIGN(FullCodegenCompilationJob);
On 2017/01/10 10:10:43, marja wrote:
> You probably didn't mean to delete the DISALLOW_...?

Done, thanks.

https://codereview.chromium.org/2618553004/diff/50001/src/zone/zone-containers.h
File src/zone/zone-containers.h (right):

https://codereview.chromium.org/2618553004/diff/50001/src/zone/zone-container...
src/zone/zone-containers.h:16: #include "src/utils.h"
On 2017/01/10 10:10:43, marja wrote:
> Why?

You are right , this wasn't needed (removed anyway since I moved the helper into
utils.h instead).

https://codereview.chromium.org/2618553004/diff/50001/src/zone/zone-container...
src/zone/zone-containers.h:145: class ThreadedListZoneEntry final : public
ZoneObject {
On 2017/01/10 12:26:09, Michael Starzinger wrote:
> This file is specifically meant to contain stdlib containers living on the
zone
> (and specifically don't subclass ZoneObject). Could we move this helper class
> into the utils.h file instead, close to our hand-woven ThreadedList class?

I had it there originally but it means src/utils.h needs to to include
src/zone/zone.h (for ZoneObject), however that created a circular dependency
since zone.h already includes src/utils.h (indirectly through src/list.h). 

It turns out list.h only needed vector.h, so I broke the cycle  and moved this
back into utils.h, however it needed a bit of include munging across the
codebase.

https://codereview.chromium.org/2618553004/diff/50001/src/zone/zone-container...
src/zone/zone-containers.h:155: };
On 2017/01/10 09:57:39, jochen wrote:
> disallow copy/assign?

Done.

[rmcilroy, 2017-01-11 10:20:42]

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

[rmcilroy, 2017-01-11 10:20:44]

+ahaas@chromium.org for asmjs changes.

[ahaas, 2017-01-11 10:23:28]

On 2017/01/11 at 10:20:44, rmcilroy wrote:
> +ahaas@chromium.org for asmjs changes.

asmjs lgtm

[Michael Starzinger, 2017-01-11 11:28:38]

LGTM from my end.

[marja, 2017-01-11 11:38:02]

I meant the LazyCompilationMode flag.

Still LGTM (didn't look at the diff in detail)

[rmcilroy, 2017-01-11 12:16:50]

On 2017/01/11 11:38:02, marja wrote:
> I meant the LazyCompilationMode flag.

Ahh I see - that flag wouldn't have let you compile eagerly though, it would
just allow always lazily compiling functions which should be compiled eagerly.

[rmcilroy, 2017-01-11 12:16:58]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2017-01-11 12:16:59]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2618553004/#ps110001
(title: "Address comments and remove field from ParseInfo")

[commit-bot: I haz the power, 2017-01-11 12:17:10]

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-11 12:18:49]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1484137018581030,
"parent_rev": "a8cfc2f5e49a40f134ac342ae0cf629df494e6ce", "commit_rev":
"a3052cfe2209420afd7d9cb621103b384d23623e"}

[commit-bot: I haz the power, 2017-01-11 12:18:54]

Description was changed from

==========
[compiler] Collect eager inner functions for compilation during renumbering.

This CL modifies the ast-numbering phase to collect function literals which
should be compiled eagerly. This is then used to eagerly compile the inner
functions before compiling the outer function. This will be used to queue
compilation jobs on the CompilerDispatcher in a later CL.

This CL moves the compilation of eager inner functions out of the
GetSharedFunctionInfo function and instead compiles them explicitly. This
simplifies GetSharedFunctionInfo and also means there is no need to pass a
LazyCompilationMode to the function, so this concept has been removed.

BUG=v8:5203,v8:5215
==========

to

==========
[compiler] Collect eager inner functions for compilation during renumbering.

This CL modifies the ast-numbering phase to collect function literals which
should be compiled eagerly. This is then used to eagerly compile the inner
functions before compiling the outer function. This will be used to queue
compilation jobs on the CompilerDispatcher in a later CL.

This CL moves the compilation of eager inner functions out of the
GetSharedFunctionInfo function and instead compiles them explicitly. This
simplifies GetSharedFunctionInfo and also means there is no need to pass a
LazyCompilationMode to the function, so this concept has been removed.

BUG=v8:5203,v8:5215

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

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

Committed patchset #5 (id:110001) as
https://chromium.googlesource.com/v8/v8/+/a3052cfe2209420afd7d9cb621103b384d2...