[TypeFeedbackVector] Root literal arrays in function literals slots

[TypeFeedbackVector] Root literal arrays in function literals slots

Literal arrays and feedback vectors for a function can be garbage
collected if we don't have a rooted closure for the function, which
happens often. It's expensive to come back from this (recreating
boilerplates and gathering feedback again), and the cost is 
disproportionate if the function was inlined into optimized code.

To guard against losing these arrays when we need them, we'll now
create literal arrays when creating the feedback vector for the outer
closure, and root them strongly in that vector.

BUG=v8:5456
Review-Url: https://codereview.chromium.org/2504153002
Cr-Commit-Position: refs/heads/master@{#41893}
Committed: https://chromium.googlesource.com/v8/v8/+/93df094081f04c629c3df2e40318de90ce5e0fb9

[mvstanton, 2016-12-07 22:20:20]

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

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

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-07 22:24:57]

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

[commit-bot: I haz the power, 2016-12-07 22:24:58]

Dry run: Try jobs failed on following builders:
  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/...)

[mvstanton, 2016-12-07 22:39:24]

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

[commit-bot: I haz the power, 2016-12-07 22:39:31]

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

[mvstanton, 2016-12-07 22:42:01]

mvstanton@chromium.org changed reviewers:
+ bmeurer@chromium.org

[mvstanton, 2016-12-07 22:42:01]

mvstanton@chromium.org changed required reviewers:
+ bmeurer@chromium.org

[mvstanton, 2016-12-07 22:42:03]

Hi Benedikt, hi Igor,
Here is the TypeFeedbackVector CL that roots the literals array for a closure in
the outer feedback vector. It seems like enough pre-requisites have landed to
make it worthwhile to check in now. Thanks for the look!

Igor, if you could pay special attention to type-feedback-vector.* and
ast.h/.cc, and verify that you are okay with the new Kind::FIXED_ARRAY or
suggest any changes.

Thanks,
--Michael

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

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

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

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_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)

[Benedikt Meurer, 2016-12-08 04:59:30]

I find this FIXED_ARRAY kind name very confusing. How about giving it a more
descriptive name like CLOSURE or CREATE_CLOSURE? We usually name the feedback
vector slots after the intended uses, i.e. CALL_IC, LOAD_IC, etc.

[mvstanton, 2016-12-08 09:58:32]

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

[commit-bot: I haz the power, 2016-12-08 09:58:41]

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-08 10:00:11]

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

[commit-bot: I haz the power, 2016-12-08 10:00:12]

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_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_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/17486)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/13893)

[mvstanton, 2016-12-08 11:27:15]

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

[commit-bot: I haz the power, 2016-12-08 11:27:24]

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-08 11:37:40]

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

[commit-bot: I haz the power, 2016-12-08 11:37:41]

Dry run: Try jobs failed on following builders:
  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...)

[mvstanton, 2016-12-08 13:45:34]

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

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

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

[mvstanton, 2016-12-08 13:47:20]

Patchset #7 (id:120001) has been deleted

[mvstanton, 2016-12-08 13:47:42]

Patchset #6 (id:100001) has been deleted

[mvstanton, 2016-12-08 13:47:57]

Patchset #7 (id:160001) has been deleted

[mvstanton, 2016-12-08 13:48:13]

Patchset #5 (id:80001) has been deleted

[mvstanton, 2016-12-08 13:48:29]

Patchset #6 (id:180001) has been deleted

[mvstanton, 2016-12-08 13:48:59]

Patchset #5 (id:140001) has been deleted

[mvstanton, 2016-12-08 13:49:16]

Patchset #5 (id:200001) has been deleted

[mvstanton, 2016-12-08 13:50:06]

Hi Benedikt,
Thanks, good advice...I changed it to CREATE_CLOSURE. 
Best!
--Michael

[commit-bot: I haz the power, 2016-12-08 13:55:50]

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

[commit-bot: I haz the power, 2016-12-08 13:55:51]

Dry run: Try jobs failed on following builders:
  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...)

[mvstanton, 2016-12-08 16:49:19]

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

[commit-bot: I haz the power, 2016-12-08 16:49:29]

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-08 16:58:44]

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

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

Dry run: Try jobs failed on following builders:
  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...)

[Benedikt Meurer, 2016-12-10 17:20:37]

Sorry, I was unable to finish a full review by now, still need to wrap my head
around most of this. Overall looks good, comments on comments.

https://codereview.chromium.org/2504153002/diff/330001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2504153002/diff/330001/src/code-stubs.cc#newc...
src/code-stubs.cc:2572: // TODO(mvstanton): Do I need a write barrier here?
A write barrier for a load?

https://codereview.chromium.org/2504153002/diff/330001/src/compiler/js-create...
File src/compiler/js-create-lowering.cc (right):

https://codereview.chromium.org/2504153002/diff/330001/src/compiler/js-create...
src/compiler/js-create-lowering.cc:772: // TODO(mvstanton): With literals
retrieved from the vector, we can
Do it :-)

[Benedikt Meurer, 2016-12-11 17:58:09]

Ok, sorry for the delay (family sickness pushed back on my work activity). I
think we should consider simplifying the literals first, then you don't need to
add the parameter machinery.

https://codereview.chromium.org/2504153002/diff/330001/src/type-feedback-vect...
File src/type-feedback-vector.cc (right):

https://codereview.chromium.org/2504153002/diff/330001/src/type-feedback-vect...
src/type-feedback-vector.cc:238: for (int i = 0; i < slot_count;) {
Hm, this looks expensive. Did you measure the perf impact?

https://codereview.chromium.org/2504153002/diff/330001/src/type-feedback-vect...
File src/type-feedback-vector.h (right):

https://codereview.chromium.org/2504153002/diff/330001/src/type-feedback-vect...
src/type-feedback-vector.h:63: This()->append_parameter(size);
This parameter business looks a bit complicated for this purpose to me.

We talked about moving the literals to the type feedback vector and essentially
turning this indirection into a Cell or Struct. How about doing that first, then
you don't need the parameters at all?

[Benedikt Meurer, 2016-12-19 10:53:22]

lgtm

[mvstanton, 2016-12-19 14:24:29]

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

[commit-bot: I haz the power, 2016-12-19 14:24:38]

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-19 14:35:08]

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

[commit-bot: I haz the power, 2016-12-19 14:35:10]

Dry run: Try jobs failed on following builders:
  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...)

[mvstanton, 2016-12-21 11:58:06]

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

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

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-21 12:29:35]

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

[commit-bot: I haz the power, 2016-12-21 12:29:36]

Dry run: This issue passed the CQ dry run.

[mvstanton, 2016-12-21 12:58:02]

Description was changed from

==========
[TypeFeedbackVector] Allow creation of a fixed array in a slot

To support creating a strong root for literals of an inner closure, we'd
like to be able to create the literals array rooted in the feedback
vector. This means being able to decide at parse time the array
you want to create for the literals, and at vector creation time, to
actually create that array.

Flags strong_rooted_literals and trace_strong_rooted_literals control
and debug this feature.

BUG=
==========

to

==========
[TypeFeedbackVector] Root literal arrays in function literals slots

Literal arrays and feedback vectors for a function can be garbage collected if
we don't have a rooted closure for the function, which happens often. It's
expensive to come back from this (recreating boilerplates and gathering feedback
again), and the cost is disproportionate if the function was inlined into
optimized code.

To guard against losing these arrays when we need them, we'll now create literal
arrays when creating the feedback vector for the outer closure, and root them
strongly in that vector.

BUG=v8:5456
==========

[mvstanton, 2016-12-21 12:58:49]

Description was changed from

==========
[TypeFeedbackVector] Root literal arrays in function literals slots

Literal arrays and feedback vectors for a function can be garbage collected if
we don't have a rooted closure for the function, which happens often. It's
expensive to come back from this (recreating boilerplates and gathering feedback
again), and the cost is disproportionate if the function was inlined into
optimized code.

To guard against losing these arrays when we need them, we'll now create literal
arrays when creating the feedback vector for the outer closure, and root them
strongly in that vector.

BUG=v8:5456
==========

to

==========
[TypeFeedbackVector] Root literal arrays in function literals slots

Literal arrays and feedback vectors for a function can be garbage
collected if we don't have a rooted closure for the function, which
happens often. It's expensive to come back from this (recreating
boilerplates and gathering feedback again), and the cost is 
disproportionate if the function was inlined into optimized code.

To guard against losing these arrays when we need them, we'll now
create literal arrays when creating the feedback vector for the outer
closure, and root them strongly in that vector.

BUG=v8:5456
==========

[mvstanton, 2016-12-21 12:59:09]

Patchset #8 (id:230064) has been deleted

[mvstanton, 2016-12-21 12:59:27]

Patchset #8 (id:290001) has been deleted

[mvstanton, 2016-12-21 13:09:14]

Thanks Benedikt, comments below. We'll push forward on unifying the vector and
literals array soon...thx again :)
--Michael

https://codereview.chromium.org/2504153002/diff/330001/src/code-stubs.cc
File src/code-stubs.cc (right):

https://codereview.chromium.org/2504153002/diff/330001/src/code-stubs.cc#newc...
src/code-stubs.cc:2572: // TODO(mvstanton): Do I need a write barrier here?
On 2016/12/10 17:20:36, Benedikt Meurer wrote:
> A write barrier for a load?

Ach, sorry the todo is for the store of the literals array in the new object
just below. I believe I don't need it.

https://codereview.chromium.org/2504153002/diff/330001/src/compiler/js-create...
File src/compiler/js-create-lowering.cc (right):

https://codereview.chromium.org/2504153002/diff/330001/src/compiler/js-create...
src/compiler/js-create-lowering.cc:772: // TODO(mvstanton): With literals
retrieved from the vector, we can
On 2016/12/10 17:20:36, Benedikt Meurer wrote:
> Do it :-)

The CL is bursting at the seams, so I'll wait (but eagerly address it!) :)

https://codereview.chromium.org/2504153002/diff/330001/src/type-feedback-vect...
File src/type-feedback-vector.cc (right):

https://codereview.chromium.org/2504153002/diff/330001/src/type-feedback-vect...
src/type-feedback-vector.cc:238: for (int i = 0; i < slot_count;) {
On 2016/12/11 17:58:09, Benedikt Meurer wrote:
> Hm, this looks expensive. Did you measure the perf impact?

The performance does seem to be okay.

https://codereview.chromium.org/2504153002/diff/330001/src/type-feedback-vect...
File src/type-feedback-vector.h (right):

https://codereview.chromium.org/2504153002/diff/330001/src/type-feedback-vect...
src/type-feedback-vector.h:63: This()->append_parameter(size);
On 2016/12/11 17:58:09, Benedikt Meurer wrote:
> This parameter business looks a bit complicated for this purpose to me.
> 
> We talked about moving the literals to the type feedback vector and
essentially
> turning this indirection into a Cell or Struct. How about doing that first,
then
> you don't need the parameters at all?

Good point. However just this change (rooting the array in the vector, adapting
CreateNewClosure in all it's iterations to do the right thing, adapting
CompileLazy stub, etc.) is really large. Merging the literal array and the
feedback vector into a single structure is a big job of it's own. I think we can
do that next.

Right now we are benefiting from an asymmetry here: the literals array can be
created after parse, but the vector can't be created until compile. This fact
allows us to root the literal array and pass it around for lazy compilation,
rather than having to remember the *location* where the literal array is rooted,
and pass that around instead (more administration that way). A lazy compile just
inserts the feedback vector into the literals array, created long before.

[mvstanton, 2016-12-21 13:09:26]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2016-12-21 13:09:27]

The patchset sent to the CQ was uploaded after l-g-t-m from bmeurer@chromium.org
Link to the patchset: https://codereview.chromium.org/2504153002/#ps370001
(title: "REBASE.")

[commit-bot: I haz the power, 2016-12-21 13:09:39]

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-21 13:13:28]

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

[commit-bot: I haz the power, 2016-12-21 13:13:30]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/31167)

[mvstanton, 2016-12-21 13:33:12]

mvstanton@chromium.org changed reviewers:
+ mlippautz@chromium.org

[mvstanton, 2016-12-21 13:33:12]

mvstanton@chromium.org changed required reviewers:
+ mlippautz@chromium.org

[mvstanton, 2016-12-21 13:33:13]

Hi Michael,
Thanks for the discussion and quick look at heap-stats.cc.
As we discussed, since the literals are no longer cached in the optimized code
map, we won't count them in stats there...instead we'll rely on the size
calculation in JSFunctions to roll that up. Thanks again!
--Michael

[Michael Lippautz, 2016-12-21 13:59:21]

On 2016/12/21 13:33:13, mvstanton wrote:
> Hi Michael,
> Thanks for the discussion and quick look at heap-stats.cc.
> As we discussed, since the literals are no longer cached in the optimized code
> map, we won't count them in stats there...instead we'll rely on the size
> calculation in JSFunctions to roll that up. Thanks again!
> --Michael

heap/ lgtm

[mvstanton, 2016-12-21 14:03:52]

The CQ bit was checked by mvstanton@chromium.org

[commit-bot: I haz the power, 2016-12-21 14:03:58]

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-21 14:06:31]

CQ is committing da patch.

Bot data: {"patchset_id": 370001, "attempt_start_ts": 1482329032061010,
"parent_rev": "c9cb94a06fa7a863d24dd6760b66cecd55748abf", "commit_rev":
"93df094081f04c629c3df2e40318de90ce5e0fb9"}

[commit-bot: I haz the power, 2016-12-21 14:06:35]

Description was changed from

==========
[TypeFeedbackVector] Root literal arrays in function literals slots

Literal arrays and feedback vectors for a function can be garbage
collected if we don't have a rooted closure for the function, which
happens often. It's expensive to come back from this (recreating
boilerplates and gathering feedback again), and the cost is 
disproportionate if the function was inlined into optimized code.

To guard against losing these arrays when we need them, we'll now
create literal arrays when creating the feedback vector for the outer
closure, and root them strongly in that vector.

BUG=v8:5456
==========

to

==========
[TypeFeedbackVector] Root literal arrays in function literals slots

Literal arrays and feedback vectors for a function can be garbage
collected if we don't have a rooted closure for the function, which
happens often. It's expensive to come back from this (recreating
boilerplates and gathering feedback again), and the cost is 
disproportionate if the function was inlined into optimized code.

To guard against losing these arrays when we need them, we'll now
create literal arrays when creating the feedback vector for the outer
closure, and root them strongly in that vector.

BUG=v8:5456

Review-Url: https://codereview.chromium.org/2504153002
Cr-Commit-Position: refs/heads/master@{#41893}
Committed:
https://chromium.googlesource.com/v8/v8/+/93df094081f04c629c3df2e40318de90ce5...
==========

[commit-bot: I haz the power, 2016-12-21 14:06:37]

Committed patchset #11 (id:370001) as
https://chromium.googlesource.com/v8/v8/+/93df094081f04c629c3df2e40318de90ce5...

[Michael Hablich, 2016-12-22 10:26:02]

A revert of this CL (patchset #11 id:370001) has been created in
https://codereview.chromium.org/2597163002/ by hablich@chromium.org.

The reason for reverting is: Speculative revert because of blocked roll:
https://codereview.chromium.org/2596013002/.