[TypeFeedbackVector] DeclareGlobals needs a literals array

[TypeFeedbackVector] DeclareGlobals needs a literals array

[RELAND with one change: until literal arrays are rooted in the outer
feedback vector (coming in the next days), the runtime-scope.cc change
is held off.]

When a function is declared in global scope, the closure is created
by the DeclareGlobals runtime service. It needs a pointer to the
literals array, already allocated in the feedback vector. This
fixes a bug where it's behavior wasn't in sync with CreateClosure,
which accepts the literals from the vector.

This enables a follow-on performance improvement in the CompileLazy
builtin.

BUG=680637
Review-Url: https://codereview.chromium.org/2634283003
Cr-Commit-Position: refs/heads/master@{#42620}
Committed: https://chromium.googlesource.com/v8/v8/+/d287c8196990b04169d5dc4e9cf6eb03d88d9e47

[mvstanton, 2017-01-17 11:17:05]

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

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

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-17 11:45:14]

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

[commit-bot: I haz the power, 2017-01-17 11:45:15]

Dry run: This issue passed the CQ dry run.

[mvstanton, 2017-01-17 12:33:29]

Description was changed from

==========
[TypeFeedbackVector] DeclareGlobals needs to behave like CreateClosure.

For function declarations, that is. Both DeclareGlobals and
CreateClosure need to accept the vector slot where the literals array
is defined.

This enables an improvement in CompileLazy that bails out less often to
the runtime.

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

to

==========
[TypeFeedbackVector] DeclareGlobals needs function literals

When a function is declared in global scope, the closure is created
by the DeclareGlobals runtime service. It needs a pointer to the
literals array, already allocated in the feedback vector. This
fixes a bug where it's behavior wasn't in sync with CreateClosure,
which accepts the literals from the vector.

This enables a follow-on performance improvement in the CompileLazy
builtin.

BUG=681543
==========

[mvstanton, 2017-01-17 12:33:29]

mvstanton@chromium.org changed reviewers:
+ leszeks@chromium.org, mstarzinger@chromium.org

[mvstanton, 2017-01-17 12:33:29]

mvstanton@chromium.org changed required reviewers:
+ leszeks@chromium.org, mstarzinger@chromium.org

[mvstanton, 2017-01-17 12:34:09]

Hi Michi, hi Leszek,
Thanks for the look.
Leszek, if you'd just like to look at the interpreter fixes that's fine.
All the best!
--Michael

[mvstanton, 2017-01-17 12:34:18]

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

[commit-bot: I haz the power, 2017-01-17 12:34:26]

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

[Michael Starzinger, 2017-01-17 12:48:21]

LGTM from my side.

https://codereview.chromium.org/2634283003/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2634283003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:533: if
(declaration.literal_slot.IsInvalid()) {
nit: We could pull out the case distinction into a local variable (similar to
{initial_value}) to make the code easier to read. That way one would immediately
see that there are always four values being pushed.

https://codereview.chromium.org/2634283003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:566: : name(name), slot(slot), func(func)
{}
nit: Let's explicitly initialize the {literal_slot} field to
{FeedbackVectorSlot::Invalid()} in the other constructors.

[mvstanton, 2017-01-17 12:53:29]

Description was changed from

==========
[TypeFeedbackVector] DeclareGlobals needs function literals

When a function is declared in global scope, the closure is created
by the DeclareGlobals runtime service. It needs a pointer to the
literals array, already allocated in the feedback vector. This
fixes a bug where it's behavior wasn't in sync with CreateClosure,
which accepts the literals from the vector.

This enables a follow-on performance improvement in the CompileLazy
builtin.

BUG=681543
==========

to

==========
[TypeFeedbackVector] DeclareGlobals needs a literals array

When a function is declared in global scope, the closure is created
by the DeclareGlobals runtime service. It needs a pointer to the
literals array, already allocated in the feedback vector. This
fixes a bug where it's behavior wasn't in sync with CreateClosure,
which accepts the literals from the vector.

This enables a follow-on performance improvement in the CompileLazy
builtin.

BUG=681543
==========

[mvstanton, 2017-01-17 12:53:29]

mvstanton@chromium.org changed required reviewers:
- leszeks@chromium.org

[commit-bot: I haz the power, 2017-01-17 12:59:16]

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

[commit-bot: I haz the power, 2017-01-17 12:59:18]

Dry run: This issue passed the CQ dry run.

[mvstanton, 2017-01-17 13:04:39]

Okay, thanks Michael, addressed. :)

https://codereview.chromium.org/2634283003/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2634283003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:533: if
(declaration.literal_slot.IsInvalid()) {
On 2017/01/17 12:48:21, Michael Starzinger wrote:
> nit: We could pull out the case distinction into a local variable (similar to
> {initial_value}) to make the code easier to read. That way one would
immediately
> see that there are always four values being pushed.

Good idea, thanks!

https://codereview.chromium.org/2634283003/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:566: : name(name), slot(slot), func(func)
{}
On 2017/01/17 12:48:21, Michael Starzinger wrote:
> nit: Let's explicitly initialize the {literal_slot} field to
> {FeedbackVectorSlot::Invalid()} in the other constructors.

Done.

[Benedikt Meurer, 2017-01-17 13:06:00]

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

[Benedikt Meurer, 2017-01-17 13:06:00]

LGTM

[Leszek Swirski, 2017-01-17 13:06:37]

interpreter lgtm

[mvstanton, 2017-01-17 13:15:27]

Description was changed from

==========
[TypeFeedbackVector] DeclareGlobals needs a literals array

When a function is declared in global scope, the closure is created
by the DeclareGlobals runtime service. It needs a pointer to the
literals array, already allocated in the feedback vector. This
fixes a bug where it's behavior wasn't in sync with CreateClosure,
which accepts the literals from the vector.

This enables a follow-on performance improvement in the CompileLazy
builtin.

BUG=681543
==========

to

==========
[TypeFeedbackVector] DeclareGlobals needs a literals array

When a function is declared in global scope, the closure is created
by the DeclareGlobals runtime service. It needs a pointer to the
literals array, already allocated in the feedback vector. This
fixes a bug where it's behavior wasn't in sync with CreateClosure,
which accepts the literals from the vector.

This enables a follow-on performance improvement in the CompileLazy
builtin.

BUG=680637
==========

[mvstanton, 2017-01-17 13:19:36]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2017-01-17 13:19:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2634283003/#ps60001
(title: "Forgotten nit.")

[commit-bot: I haz the power, 2017-01-17 13:19:48]

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-17 13:47:01]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484659176646360,
"parent_rev": "c5948b9897fd3d2fe2cb670375eda59ff7aff0c9", "commit_rev":
"b83c3d29a95f672d16ac61b50484120e3a676d9e"}

[commit-bot: I haz the power, 2017-01-17 13:47:04]

Description was changed from

==========
[TypeFeedbackVector] DeclareGlobals needs a literals array

When a function is declared in global scope, the closure is created
by the DeclareGlobals runtime service. It needs a pointer to the
literals array, already allocated in the feedback vector. This
fixes a bug where it's behavior wasn't in sync with CreateClosure,
which accepts the literals from the vector.

This enables a follow-on performance improvement in the CompileLazy
builtin.

BUG=680637
==========

to

==========
[TypeFeedbackVector] DeclareGlobals needs a literals array

When a function is declared in global scope, the closure is created
by the DeclareGlobals runtime service. It needs a pointer to the
literals array, already allocated in the feedback vector. This
fixes a bug where it's behavior wasn't in sync with CreateClosure,
which accepts the literals from the vector.

This enables a follow-on performance improvement in the CompileLazy
builtin.

BUG=680637

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

[commit-bot: I haz the power, 2017-01-17 13:47:05]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/b83c3d29a95f672d16ac61b50484120e3a6...

[mvstanton, 2017-01-23 16:00:41]

Description was changed from

==========
[TypeFeedbackVector] DeclareGlobals needs a literals array

When a function is declared in global scope, the closure is created
by the DeclareGlobals runtime service. It needs a pointer to the
literals array, already allocated in the feedback vector. This
fixes a bug where it's behavior wasn't in sync with CreateClosure,
which accepts the literals from the vector.

This enables a follow-on performance improvement in the CompileLazy
builtin.

BUG=680637

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

to

==========
[TypeFeedbackVector] DeclareGlobals needs a literals array

When a function is declared in global scope, the closure is created
by the DeclareGlobals runtime service. It needs a pointer to the
literals array, already allocated in the feedback vector. This
fixes a bug where it's behavior wasn't in sync with CreateClosure,
which accepts the literals from the vector.

This enables a follow-on performance improvement in the CompileLazy
builtin.

BUG=680637

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

[mvstanton, 2017-01-23 16:01:46]

Description was changed from

==========
[TypeFeedbackVector] DeclareGlobals needs a literals array

When a function is declared in global scope, the closure is created
by the DeclareGlobals runtime service. It needs a pointer to the
literals array, already allocated in the feedback vector. This
fixes a bug where it's behavior wasn't in sync with CreateClosure,
which accepts the literals from the vector.

This enables a follow-on performance improvement in the CompileLazy
builtin.

BUG=680637

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

to

==========
[TypeFeedbackVector] DeclareGlobals needs a literals array

[RELAND with one change: until literal arrays are rooted in the outer
feedback vector (coming in the next days), the runtime-scope.cc change
is held off.]

When a function is declared in global scope, the closure is created
by the DeclareGlobals runtime service. It needs a pointer to the
literals array, already allocated in the feedback vector. This
fixes a bug where it's behavior wasn't in sync with CreateClosure,
which accepts the literals from the vector.

This enables a follow-on performance improvement in the CompileLazy
builtin.

BUG=680637
==========

[mvstanton, 2017-01-23 16:03:09]

Hi Michael,
To get the literal array work back in the tree, it makes sense to invert the
order of landing some CLs. This one should land first, modulo a change in one
file.

Thanks!
--Michael

[mvstanton, 2017-01-23 16:07:23]

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

[commit-bot: I haz the power, 2017-01-23 16:07: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, 2017-01-23 16:38:56]

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

[commit-bot: I haz the power, 2017-01-23 16:38:57]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2017-01-23 17:06:58]

Yep. Patch set #5 still LGTM.

[mvstanton, 2017-01-24 08:44:37]

The CQ bit was checked by mvstanton@chromium.org

[mvstanton, 2017-01-24 08:44:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, leszeks@chromium.org
Link to the patchset: https://codereview.chromium.org/2634283003/#ps80001
(title: "Don't exploit the literals array yet.")

[commit-bot: I haz the power, 2017-01-24 08:44:43]

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-24 08:46:41]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485247477300930,
"parent_rev": "09163d71bd3e1074dfa8ed439c789b67ee797133", "commit_rev":
"d287c8196990b04169d5dc4e9cf6eb03d88d9e47"}

[commit-bot: I haz the power, 2017-01-24 08:46:47]

Description was changed from

==========
[TypeFeedbackVector] DeclareGlobals needs a literals array

[RELAND with one change: until literal arrays are rooted in the outer
feedback vector (coming in the next days), the runtime-scope.cc change
is held off.]

When a function is declared in global scope, the closure is created
by the DeclareGlobals runtime service. It needs a pointer to the
literals array, already allocated in the feedback vector. This
fixes a bug where it's behavior wasn't in sync with CreateClosure,
which accepts the literals from the vector.

This enables a follow-on performance improvement in the CompileLazy
builtin.

BUG=680637
==========

to

==========
[TypeFeedbackVector] DeclareGlobals needs a literals array

[RELAND with one change: until literal arrays are rooted in the outer
feedback vector (coming in the next days), the runtime-scope.cc change
is held off.]

When a function is declared in global scope, the closure is created
by the DeclareGlobals runtime service. It needs a pointer to the
literals array, already allocated in the feedback vector. This
fixes a bug where it's behavior wasn't in sync with CreateClosure,
which accepts the literals from the vector.

This enables a follow-on performance improvement in the CompileLazy
builtin.

BUG=680637

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

[commit-bot: I haz the power, 2017-01-24 08:46:48]

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