[Interpreter] Use FastCloneShallowObjectStub in CreateObjectLiteral bytecode.

[Interpreter] Use FastCloneShallowObjectStub in CreateObjectLiteral bytecode.

Adapts FastCloneShallowObjectStub to enable it to be used by the
CreateObjectLiteral bytecode.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/ac2a17abcb2a179718816dea8ccfb11cd74b8177
Cr-Commit-Position: refs/heads/master@{#35909}

[rmcilroy, 2016-04-25 14:14:30]

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

[commit-bot: I haz the power, 2016-04-25 14:14:37]

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

[commit-bot: I haz the power, 2016-04-25 14:29:27]

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

[commit-bot: I haz the power, 2016-04-25 14:29:28]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[rmcilroy, 2016-04-25 15:42:06]

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

[commit-bot: I haz the power, 2016-04-25 15:42:12]

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

[rmcilroy, 2016-04-25 15:42:15]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-04-25 16:17:39]

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

[commit-bot: I haz the power, 2016-04-25 16:17:40]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-04-26 09:14:53]

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

[rmcilroy, 2016-04-26 09:14:54]

Toon, this is an attempt to make FastCloneShallowObjectStub usable by the
interpreter. I'm inlining the fast-path case into the bytecode handler since
otherwise I would need to pass an additional parameter to the stub for
object-size (which is dynamic in the interpreter, unlike the code-stub which is
generated specifically for a given object size). Let me know what you think, if
it seems reasonable I'll add other folks onto the review to review the other
bits.

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

https://codereview.chromium.org/1922523002/diff/20001/src/code-stubs.cc#newco...
src/code-stubs.cc:3866: {
I've made this boilerplate copy code a loop for both the cases where the
object_size is statically known (the code stubs) as well as when it is provided
dynamically (ignition) to keep things simpler. I can add back the statically
unrolled loop for the code-stubs case if you think it would be worth it from a
performance point of view. WDYT?

[Toon Verwaest, 2016-04-27 08:50:20]

lgtm

https://codereview.chromium.org/1922523002/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1922523002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1458: __ BranchIf(fast_clone_properties_count,
&if_fast_clone, &if_not_fast_clone);
Does this flag change over time? If not, why not have 2 different bytecodes
where one always calls the runtime and the other always uses the fast stub?

https://codereview.chromium.org/1922523002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1465: __ WordShl(fast_clone_properties_count,
kPointerSizeLog2);
What about just passing in fast_clone_properties_count and letting
GenerateFastPath calculate the object size. For the stubs we'd just rely on
constant folding.

[rmcilroy, 2016-04-28 13:30:51]

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

[rmcilroy, 2016-04-28 13:30:52]

Orion: PTAL at interpreter changes
Michi: PTAL at code-stub-assembler and compiler. changes.

https://codereview.chromium.org/1922523002/diff/40001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1922523002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1458: __ BranchIf(fast_clone_properties_count,
&if_fast_clone, &if_not_fast_clone);
On 2016/04/27 08:50:19, Toon Verwaest wrote:
> Does this flag change over time? If not, why not have 2 different bytecodes
> where one always calls the runtime and the other always uses the fast stub?

I considered this, but I didn't think it was worth the extra bytecode given that
it is only one extra check here on the fastpath, and the fast path does two
other checks which cause it to bailout to the runtime anyway.

https://codereview.chromium.org/1922523002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter.cc:1465: __ WordShl(fast_clone_properties_count,
kPointerSizeLog2);
On 2016/04/27 08:50:19, Toon Verwaest wrote:
> What about just passing in fast_clone_properties_count and letting
> GenerateFastPath calculate the object size. For the stubs we'd just rely on
> constant folding.

Works for me, done.

[oth, 2016-04-28 14:17:15]

lgtm on interpreter, thanks.

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

https://codereview.chromium.org/1922523002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1550: STATIC_ASSERT(
This assertion might be better placed in the encoding routine.

https://codereview.chromium.org/1922523002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1557:
CreateObjectLiteralFlags::FlagsBits::encode(expr->ComputeFlags()) |
The code could DCHECK here that the components of flags are orthogonal.

[Michael Starzinger, 2016-04-29 08:37:31]

LGTM.

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

https://codereview.chromium.org/1922523002/diff/60001/src/code-stubs.cc#newco...
src/code-stubs.cc:3820: int FastCloneShallowObjectStub::PropertiesCount(int
literal_length) {
Non-actionable question just for my understanding: This properties count looks
specific to the fast-clone stub. Is the runtime using the same heuristic for
choosing the initial properties count? If so, is there a plan to eventually
unify these heuristics or will this computation always be specific to the
fast-clone stub?

https://codereview.chromium.org/1922523002/diff/60001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1922523002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter.cc:29: typedef compiler::CodeAssembler::Label Label;
nit: Shouldn't this be covered by the typedefs a couple of lines above?

[Toon Verwaest, 2016-04-29 09:07:10]

lgtm

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

https://codereview.chromium.org/1922523002/diff/60001/src/code-stubs.cc#newco...
src/code-stubs.cc:3820: int FastCloneShallowObjectStub::PropertiesCount(int
literal_length) {
On 2016/04/29 08:37:31, Michael Starzinger wrote:
> Non-actionable question just for my understanding: This properties count looks
> specific to the fast-clone stub. Is the runtime using the same heuristic for
> choosing the initial properties count? If so, is there a plan to eventually
> unify these heuristics or will this computation always be specific to the
> fast-clone stub?

Yes, they are the same; and they have to be the same. This is what we discussed
before. We should unify them indeed.

[Michael Starzinger, 2016-04-29 09:51:37]

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

https://codereview.chromium.org/1922523002/diff/60001/src/code-stubs.cc#newco...
src/code-stubs.cc:3820: int FastCloneShallowObjectStub::PropertiesCount(int
literal_length) {
On 2016/04/29 09:07:10, Toon Verwaest wrote:
> On 2016/04/29 08:37:31, Michael Starzinger wrote:
> > Non-actionable question just for my understanding: This properties count
looks
> > specific to the fast-clone stub. Is the runtime using the same heuristic for
> > choosing the initial properties count? If so, is there a plan to eventually
> > unify these heuristics or will this computation always be specific to the
> > fast-clone stub?
> 
> Yes, they are the same; and they have to be the same. This is what we
discussed
> before. We should unify them indeed.

Acknowledged. Thanks, makes sense. +1 on eventual unification.

[rmcilroy, 2016-04-29 13:53:21]

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

https://codereview.chromium.org/1922523002/diff/60001/src/code-stubs.cc#newco...
src/code-stubs.cc:3820: int FastCloneShallowObjectStub::PropertiesCount(int
literal_length) {
On 2016/04/29 09:51:36, Michael Starzinger wrote:
> On 2016/04/29 09:07:10, Toon Verwaest wrote:
> > On 2016/04/29 08:37:31, Michael Starzinger wrote:
> > > Non-actionable question just for my understanding: This properties count
> looks
> > > specific to the fast-clone stub. Is the runtime using the same heuristic
for
> > > choosing the initial properties count? If so, is there a plan to
eventually
> > > unify these heuristics or will this computation always be specific to the
> > > fast-clone stub?
> > 
> > Yes, they are the same; and they have to be the same. This is what we
> discussed
> > before. We should unify them indeed.
> 
> Acknowledged. Thanks, makes sense. +1 on eventual unification.

Ack. Added a TODO towards unification.

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

https://codereview.chromium.org/1922523002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1550: STATIC_ASSERT(
On 2016/04/28 14:17:15, oth wrote:
> This assertion might be better placed in the encoding routine.

I can't put it in the encode routine since that is generic for all BitField
generic classes. There is a DCHECK in BitField the Encode class that the value
fits in the bitfield, but that might miss something if our test cases aren't
wide enough so I thought it best to add this static assert too.

https://codereview.chromium.org/1922523002/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:1557:
CreateObjectLiteralFlags::FlagsBits::encode(expr->ComputeFlags()) |
On 2016/04/28 14:17:15, oth wrote:
> The code could DCHECK here that the components of flags are orthogonal.

Good point. Switched to using kNext in the definition of the flags, which should
ensure they are orthogonal, and added a STATIC_ASSERT in
CreateObjectLiteralFlags instead of a DCHECK here.

https://codereview.chromium.org/1922523002/diff/60001/src/interpreter/interpr...
File src/interpreter/interpreter.cc (right):

https://codereview.chromium.org/1922523002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter.cc:29: typedef compiler::CodeAssembler::Label Label;
On 2016/04/29 08:37:31, Michael Starzinger wrote:
> nit: Shouldn't this be covered by the typedefs a couple of lines above?

Yes thanks. Rebase relic from before those lines were added. Removed.

[rmcilroy, 2016-04-29 13:54:51]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2016-04-29 13:54:52]

The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org,
verwaest@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1922523002/#ps100001
(title: "Add todo")

[commit-bot: I haz the power, 2016-04-29 13:55:02]

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

[commit-bot: I haz the power, 2016-04-29 14:19:27]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-05-01 22:03:55]

Description was changed from

==========
[Interpreter] Use FastCloneShallowObjectStub in CreateObjectLiteral bytecode.

Adapts FastCloneShallowObjectStub to enable it to be used by the
CreateObjectLiteral bytecode.

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

to

==========
[Interpreter] Use FastCloneShallowObjectStub in CreateObjectLiteral bytecode.

Adapts FastCloneShallowObjectStub to enable it to be used by the
CreateObjectLiteral bytecode.

BUG=v8:4280
LOG=N

Committed: https://crrev.com/ac2a17abcb2a179718816dea8ccfb11cd74b8177
Cr-Commit-Position: refs/heads/master@{#35909}
==========

[commit-bot: I haz the power, 2016-05-01 22:03:56]

Patchset 5 (id:??) landed as
https://crrev.com/ac2a17abcb2a179718816dea8ccfb11cd74b8177
Cr-Commit-Position: refs/heads/master@{#35909}