[Interpreter] Enable allocation site mementos in CreateArrayLiterals.

[Interpreter] Enable allocation site mementos in CreateArrayLiterals.

In ignition, allocation site mementos were disabled when creating array
literals. Enabled them in this cl.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/119f311245e09bd7515697d27921fd60d9e58424
Cr-Commit-Position: refs/heads/master@{#39234}

[mythria, 2016-09-01 10:01:24]

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

[commit-bot: I haz the power, 2016-09-01 10:01: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, 2016-09-01 10:13:18]

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

[commit-bot: I haz the power, 2016-09-01 10:13: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_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[mythria, 2016-09-01 11:15:46]

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

[commit-bot: I haz the power, 2016-09-01 11:15:53]

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-09-01 11:35:13]

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

[commit-bot: I haz the power, 2016-09-01 11:35:14]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-09-02 14:46:52]

Description was changed from

==========
[Interpreter] Enable allocation site memetos. [TESTING ON BOTS]

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

to

==========
[Interpreter] Enable allocation site mementos in CreateArrayLiterals.

In ignition, allocation site mementos were disabled when creating array
literals. Enabled them in this cl.

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

[mythria, 2016-09-02 14:47:07]

Description was changed from

==========
[Interpreter] Enable allocation site mementos in CreateArrayLiterals.

In ignition, allocation site mementos were disabled when creating array
literals. Enabled them in this cl.

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

to

==========
[Interpreter] Enable allocation site mementos in CreateArrayLiterals.

In ignition, allocation site mementos were disabled when creating array
literals.
Enabled them in this cl.

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

[mythria, 2016-09-02 14:47:22]

Description was changed from

==========
[Interpreter] Enable allocation site mementos in CreateArrayLiterals.

In ignition, allocation site mementos were disabled when creating array
literals.
Enabled them in this cl.

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

to

==========
[Interpreter] Enable allocation site mementos in CreateArrayLiterals.

In ignition, allocation site mementos were disabled when creating array
literals. Enabled them in this cl.

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

[mythria, 2016-09-02 15:06:53]

mythria@chromium.org changed reviewers:
+ klaasb@google.com, rmcilroy@chromium.org

[mythria, 2016-09-02 15:06:54]

PTAL.

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

https://codereview.chromium.org/2294913006/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1886: expr->ComputeFlags(true));
Ross, I think this is the only change that is required for enabling mementos. We
call in to the runtime which already handles it. Was there any reason we
disabled it earlier? When Klass finishes porting the FastCloneShallowObjectStub
I think it should still work.

[rmcilroy, 2016-09-05 09:28:32]

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

[rmcilroy, 2016-09-05 09:28:33]

Looks good, thanks for the change. One question for Michi before we land.

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

https://codereview.chromium.org/2294913006/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1886: expr->ComputeFlags(true));
On 2016/09/02 15:06:53, mythria wrote:
> Ross, I think this is the only change that is required for enabling mementos.
We
> call in to the runtime which already handles it. Was there any reason we
> disabled it earlier? When Klass finishes porting the
FastCloneShallowObjectStub
> I think it should still work.

AST Graph Builder doesn't create mementos (presumably because we don't need to
collect any more feedback once we are in optimized code). Maybe we need to
explicitly prevent mementos in BytecodeGraphBuilder to be consistent with
ASTGraphBuilder. 

Michi, what do you think?

[Michael Starzinger, 2016-09-05 12:23:03]

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

[Michael Starzinger, 2016-09-05 12:23:04]

Quick reply, doing review next.

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

https://codereview.chromium.org/2294913006/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1886: expr->ComputeFlags(true));
On 2016/09/05 09:28:33, rmcilroy wrote:
> On 2016/09/02 15:06:53, mythria wrote:
> > Ross, I think this is the only change that is required for enabling
mementos.
> We
> > call in to the runtime which already handles it. Was there any reason we
> > disabled it earlier? When Klass finishes porting the
> FastCloneShallowObjectStub
> > I think it should still work.
> 
> AST Graph Builder doesn't create mementos (presumably because we don't need to
> collect any more feedback once we are in optimized code). Maybe we need to
> explicitly prevent mementos in BytecodeGraphBuilder to be consistent with
> ASTGraphBuilder. 
> 
> Michi, what do you think?

Yes, correct. The reasoning was that only the unoptimized code would collect
feedback and once we optimize the decision was "settled". We decided to go with
this approach until we'd see data to the contrary. Being consistent for now and
preventing creation of mementos in the BytecodeGraphBuilder sounds reasonable to
me (with a comment, explaining exactly this). I also CCed Michael Stanton about
this.

[Michael Starzinger, 2016-09-05 12:26:29]

LGTM modulo Ross' comment. I would be fine with just removing the
"ArrayLiteral::kDisableMementos" bit from the flags in
"BytecodeGraphBuilder::VisitCreateArrayLiteral" with a lengthy comment
explaining why it's done.

[mythria, 2016-09-05 13:22:04]

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

[commit-bot: I haz the power, 2016-09-05 13:22:17]

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-09-05 13:24:26]

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

[commit-bot: I haz the power, 2016-09-05 13:24:27]

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

[mythria, 2016-09-05 13:40:03]

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

[commit-bot: I haz the power, 2016-09-05 13:40:13]

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-09-05 14:07:31]

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

[commit-bot: I haz the power, 2016-09-05 14:07:31]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-09-05 15:31:54]

Thanks for the feedback. I updated bytecode-graph-builder and also added a
comment. PTAL.

Thanks,
Mythri

[rmcilroy, 2016-09-05 15:41:17]

lgtm

[Michael Starzinger, 2016-09-05 15:46:52]

LGTM. Thanks!

[mvstanton, 2016-09-07 08:14:57]

LGTM

[mythria, 2016-09-07 08:16:26]

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

[commit-bot: I haz the power, 2016-09-07 08:16:28]

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-09-07 08:53:18]

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

[commit-bot: I haz the power, 2016-09-07 08:53:20]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-09-07 09:02:57]

The CQ bit was checked by mythria@chromium.org

[commit-bot: I haz the power, 2016-09-07 09:03:08]

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-09-07 09:05:49]

Description was changed from

==========
[Interpreter] Enable allocation site mementos in CreateArrayLiterals.

In ignition, allocation site mementos were disabled when creating array
literals. Enabled them in this cl.

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

to

==========
[Interpreter] Enable allocation site mementos in CreateArrayLiterals.

In ignition, allocation site mementos were disabled when creating array
literals. Enabled them in this cl.

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

[commit-bot: I haz the power, 2016-09-07 09:05:50]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-07 09:06:26]

Description was changed from

==========
[Interpreter] Enable allocation site mementos in CreateArrayLiterals.

In ignition, allocation site mementos were disabled when creating array
literals. Enabled them in this cl.

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

to

==========
[Interpreter] Enable allocation site mementos in CreateArrayLiterals.

In ignition, allocation site mementos were disabled when creating array
literals. Enabled them in this cl.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/119f311245e09bd7515697d27921fd60d9e58424
Cr-Commit-Position: refs/heads/master@{#39234}
==========

[commit-bot: I haz the power, 2016-09-07 09:06:27]

Patchset 4 (id:??) landed as
https://crrev.com/119f311245e09bd7515697d27921fd60d9e58424
Cr-Commit-Position: refs/heads/master@{#39234}