Pretenure call new support.

Pretenure call new support.

When FLAG_pretenure_call_new is on, we emit mementos on new object creation
in full code, and consume the feedback in crankshaft. A key difference in the
generated code for stubs is the allocation of an additional type vector slot for the
CallNew AST node, which simplifies the CallConstructStub and CallFunctionStub
considerably.

Some performance tuning still needs to be addressed, therefore the flag is off at
this moment, though fully functional. The goal is to remove the flag as soon as
possible, which allows much code deletion (yay).

R=hpayer@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=20076

[mvstanton, 2014-02-11 09:16:19]

Hi Hannes, here is the pretenure call new support we've been discussing. Now we
create an allocation site at full codegen time for every call new, rather than
deferring this to first call time, which is more complex.

[Hannes Payer (out of office), 2014-02-11 15:51:22]

Super cool. Please add more tests (we could add them to cctest test-mementos.cc)
and platform ports.

https://codereview.chromium.org/132963012/diff/60001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/132963012/diff/60001/src/hydrogen.cc#newcode8241
src/hydrogen.cc:8241: if (!FLAG_allocation_site_pretenuring) {
Can we re-order this combination of cases to:
if (FLAG_pretenuring_call_new)
  if (FLAG_allocation_site_pretenuring)
  else

I think this improves readability.

https://codereview.chromium.org/132963012/diff/60001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/132963012/diff/60001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:139: // Should never create mementos for api
functions. (true?)
probably

https://codereview.chromium.org/132963012/diff/60001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:234: __ bind(&do_store);
We could also undo the Memento allocation. WDYT?

https://codereview.chromium.org/132963012/diff/60001/src/type-info.cc
File src/type-info.cc (right):

https://codereview.chromium.org/132963012/diff/60001/src/type-info.cc#newcode155
src/type-info.cc:155: Handle<Object> info = GetInfo(slot);
we can do that in one line

https://codereview.chromium.org/132963012/diff/60001/src/type-info.cc#newcode161
src/type-info.cc:161: Handle<Object> info = GetInfo(slot);
we can do that in one line

https://codereview.chromium.org/132963012/diff/60001/src/type-info.cc#newcode167
src/type-info.cc:167: Handle<Object> info = GetInfo(slot);
we can do that in one line

[mvstanton, 2014-02-17 15:53:08]

Hi Hannes,
I added some tests (they were helpful already!), and comment response. The
JSConstructStubHelper changed a fair bit, and the differences between
architectures are significant. ARM favors a "post increment" style of filling in
fields, for example. 

Thanks again for the look,
--Michael

https://codereview.chromium.org/132963012/diff/60001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/132963012/diff/60001/src/hydrogen.cc#newcode8241
src/hydrogen.cc:8241: if (!FLAG_allocation_site_pretenuring) {
On 2014/02/11 15:51:23, Hannes Payer wrote:
> Can we re-order this combination of cases to:
> if (FLAG_pretenuring_call_new)
>   if (FLAG_allocation_site_pretenuring)
>   else
> 
> I think this improves readability.

very true, thx!

https://codereview.chromium.org/132963012/diff/60001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/132963012/diff/60001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:139: // Should never create mementos for api
functions. (true?)
On 2014/02/11 15:51:23, Hannes Payer wrote:
> probably

Done.

https://codereview.chromium.org/132963012/diff/60001/src/ia32/builtins-ia32.c...
src/ia32/builtins-ia32.cc:234: __ bind(&do_store);
On 2014/02/11 15:51:23, Hannes Payer wrote:
> We could also undo the Memento allocation. WDYT?

Good point, I'll go ahead and fix it to only increment that count below the
label allocated, and to make sure that if the runtime version was called we step
over that (because the runtime routine will increment the count).

https://codereview.chromium.org/132963012/diff/60001/src/type-info.cc
File src/type-info.cc (right):

https://codereview.chromium.org/132963012/diff/60001/src/type-info.cc#newcode155
src/type-info.cc:155: Handle<Object> info = GetInfo(slot);
On 2014/02/11 15:51:23, Hannes Payer wrote:
> we can do that in one line

Done.

https://codereview.chromium.org/132963012/diff/60001/src/type-info.cc#newcode161
src/type-info.cc:161: Handle<Object> info = GetInfo(slot);
On 2014/02/11 15:51:23, Hannes Payer wrote:
> we can do that in one line

Done.

https://codereview.chromium.org/132963012/diff/60001/src/type-info.cc#newcode167
src/type-info.cc:167: Handle<Object> info = GetInfo(slot);
On 2014/02/11 15:51:23, Hannes Payer wrote:
> we can do that in one line

Done.

[Hannes Payer (out of office), 2014-02-18 16:24:25]

Mainly minor comments, looking good.

https://codereview.chromium.org/132963012/diff/210001/src/a64/builtins-a64.cc
File src/a64/builtins-a64.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/a64/builtins-a64.cc...
src/a64/builtins-a64.cc:583: __ bind(&count_incremented);
bind outside if?

https://codereview.chromium.org/132963012/diff/210001/src/a64/code-stubs-a64.cc
File src/a64/code-stubs-a64.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/a64/code-stubs-a64....
src/a64/code-stubs-a64.cc:5388: // We should either have undefined in ebx or a
valid AllocationSite
x10

https://codereview.chromium.org/132963012/diff/210001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/arm/builtins-arm.cc...
src/arm/builtins-arm.cc:498: }
Is it possible to keep 
__ add(r0, r4, Operand(r3, LSL, kPointerSizeLog2));  // End of object.	
__ InitializeFieldsWithFiller(r5, r0, r6);

https://codereview.chromium.org/132963012/diff/210001/src/arm/builtins-arm.cc...
src/arm/builtins-arm.cc:635: }
bind outside if?

https://codereview.chromium.org/132963012/diff/210001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:5276: // We should either have undefined in ebx or a
valid AllocationSite
r2

https://codereview.chromium.org/132963012/diff/210001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/factory.cc#newcode1318
src/factory.cc:1318: isolate()->heap()->AllocateJSObjectWithAllocationSite(
The refactoring of AllocateJSObject* should happen in a differen CL. I think we
should keep separate factory methods.

https://codereview.chromium.org/132963012/diff/210001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8224: if (!allocation_site.is_null()) {
When is the allocation_site null?

https://codereview.chromium.org/132963012/diff/210001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/132963012/diff/210001/src/hydrogen.h#newcode1002
src/hydrogen.h:1002: : current_site_(NULL), pretenure_flag_(NOT_TENURED) {}
indent

https://codereview.chromium.org/132963012/diff/210001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/ia32/builtins-ia32....
src/ia32/builtins-ia32.cc:268: __ InitializeFieldsWithFiller(ecx, edi, edx);
Should we keep this one out of the if/else construct, the way it was before?

https://codereview.chromium.org/132963012/diff/210001/src/ia32/builtins-ia32....
src/ia32/builtins-ia32.cc:399: __ bind(&count_incremented);
Can you move the bind outside the if case?

https://codereview.chromium.org/132963012/diff/210001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/runtime.cc#newcode8304
src/runtime.cc:8304: // TODO(mvstanton): make this a real site
ambiguous comment

https://codereview.chromium.org/132963012/diff/210001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:272: }
__ InitializeFieldsWithFiller(rcx, rdi, rdx); outside the if/else?

https://codereview.chromium.org/132963012/diff/210001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:404: }
__ bind(&count_incremented); outside of if?

https://codereview.chromium.org/132963012/diff/210001/src/x64/macro-assembler...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/x64/macro-assembler...
src/x64/macro-assembler-x64.cc:4052: bind(&fine);
I don't think this code is needed...

https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2533: "  return this;"
remove return this;

https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2579: // Test that a form of call-new pretenuring works
even without allocation sites.
Test global pretenuring of call new.

https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-mement...
File test/cctest/test-mementos.cc (right):

https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-mement...
test/cctest/test-mementos.cc:86: "a;";
a == return a; ?

[mvstanton, 2014-02-19 08:40:25]

Thanks Hannes, here are comment results.
--Michael

https://codereview.chromium.org/132963012/diff/210001/src/a64/builtins-a64.cc
File src/a64/builtins-a64.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/a64/builtins-a64.cc...
src/a64/builtins-a64.cc:478: if (create_memento) {
Hannes, I've changed this snippet of code to fit the same pattern as the other
three platforms:

if (count_constructions) {
  // object filling code for this case.
} else if (create_memento) {
  // object filling code for this case.
} else {
  // simplest case for filling object fields.
}

There is a line or two of duplication, but I think it's worth having a uniform
logical structure. In some platforms it's "not quite" duplication, like we'll
use register ESI instead of EDI in some operands in order to save an instruction
elsewhere.

https://codereview.chromium.org/132963012/diff/210001/src/a64/code-stubs-a64.cc
File src/a64/code-stubs-a64.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/a64/code-stubs-a64....
src/a64/code-stubs-a64.cc:5388: // We should either have undefined in ebx or a
valid AllocationSite
On 2014/02/18 16:24:26, Hannes Payer wrote:
> x10

Done.

https://codereview.chromium.org/132963012/diff/210001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:5276: // We should either have undefined in ebx or a
valid AllocationSite
On 2014/02/18 16:24:26, Hannes Payer wrote:
> r2

Done.

https://codereview.chromium.org/132963012/diff/210001/src/factory.cc
File src/factory.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/factory.cc#newcode1318
src/factory.cc:1318: isolate()->heap()->AllocateJSObjectWithAllocationSite(
On 2014/02/18 16:24:26, Hannes Payer wrote:
> The refactoring of AllocateJSObject* should happen in a differen CL. I think
we
> should keep separate factory methods.

Right on, I've delivered a CL to you with that refactoring, good idea.

https://codereview.chromium.org/132963012/diff/210001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8224: if (!allocation_site.is_null()) {
On 2014/02/18 16:24:26, Hannes Payer wrote:
> When is the allocation_site null?

Wow. Never. Simplifying this. In the past it was more like, "if the call new
site never went megamorphic then it could be null." However, we only come down
this code path anywhere if we are monomorphic.

BTW, two lines in type-info.cc could be simplified,
TypeFeedbackOracle::CallIsMonomorphic() and
TypeFeedbackOracle::CallNewIsMonomorphic(). Those calls were accepting an
AllocationSite in their type vector slot, but now we've separated it out into a
different slot.

https://codereview.chromium.org/132963012/diff/210001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/132963012/diff/210001/src/hydrogen.h#newcode1002
src/hydrogen.h:1002: : current_site_(NULL), pretenure_flag_(NOT_TENURED) {}
On 2014/02/18 16:24:26, Hannes Payer wrote:
> indent

Done.

https://codereview.chromium.org/132963012/diff/210001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/runtime.cc#newcode8304
src/runtime.cc:8304: // TODO(mvstanton): make this a real site
On 2014/02/18 16:24:26, Hannes Payer wrote:
> ambiguous comment

Done.

https://codereview.chromium.org/132963012/diff/210001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:272: }
On 2014/02/18 16:24:26, Hannes Payer wrote:
> __ InitializeFieldsWithFiller(rcx, rdi, rdx); outside the if/else?

It's awkward to do it because in the create_memento case
it's called with the arg RSI instead of RDI. (ia32 looks similar).

In general this "filling object fields" code was tricky enough that I finally
opted for a single logical structure to lay out the code at the expense of some
duplication/near-duplication.

https://codereview.chromium.org/132963012/diff/210001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:404: }
On 2014/02/18 16:24:26, Hannes Payer wrote:
> __ bind(&count_incremented); outside of if?

I thought about that a lot, and decided to put it inside because the only
"jumper" to that target is in a "if (create_memento)", and this means that any
other unsanctioned jumper to that address would get a fast crash at compile
time. By sticking it inside that block, someone without create_memento == true
cannot possibly jump to it and succeed in compiling this code. We compile it at
snapshot time so it's a fast/useful error.

I saw this pattern in other places...I agree it's weird but it's safer.

https://codereview.chromium.org/132963012/diff/210001/src/x64/macro-assembler...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/132963012/diff/210001/src/x64/macro-assembler...
src/x64/macro-assembler-x64.cc:4052: bind(&fine);
On 2014/02/18 16:24:26, Hannes Payer wrote:
> I don't think this code is needed...

Whoa, thank you, that was me debugging somewhere in there :).

https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2533: "  return this;"
On 2014/02/18 16:24:26, Hannes Payer wrote:
> remove return this;

Done.

https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2579: // Test that a form of call-new pretenuring works
even without allocation sites.
On 2014/02/18 16:24:26, Hannes Payer wrote:
> Test global pretenuring of call new.

Done.

https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-mement...
File test/cctest/test-mementos.cc (right):

https://codereview.chromium.org/132963012/diff/210001/test/cctest/test-mement...
test/cctest/test-mementos.cc:86: "a;";
On 2014/02/18 16:24:26, Hannes Payer wrote:
> a == return a; ?

Done.

[mvstanton, 2014-02-19 13:18:51]

Hi Hannes, here is the delta where pretenure ratio is raised to 85% to address
the Earley issue. Very interesting case...
--Michael

[Hannes Payer (out of office), 2014-02-20 08:02:24]

LGTM

[mvstanton, 2014-03-19 13:39:27]

Committed patchset #11 manually as r20076 (presubmit successful).