Create AllocationSite objects, pointed to by AllocationSiteInfo.

Create AllocationSite objects, pointed to by AllocationSiteInfo.
This creates a platform where we can do additional things with allocation sites,
other than just aid in reducing array transitions.


BUG=
R=hpayer@chromium.org

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

[mvstanton, 2013-05-28 16:12:16]

Hi Hannes, here is the initial version.

[Hannes Payer (out of office), 2013-06-25 16:35:07]

First round of comments, cool stuff!

https://codereview.chromium.org/15094018/diff/17001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/15094018/diff/17001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:320: AddInstruction(new(zone) HLoadKeyed(literals,
literals is just used there, you could just use the original code.

https://codereview.chromium.org/15094018/diff/17001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/15094018/diff/17001/src/flag-definitions.h#ne...
src/flag-definitions.h:270: DEFINE_bool(optimize_constructed_arrays, false,
Should be true.

https://codereview.chromium.org/15094018/diff/17001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/15094018/diff/17001/src/hydrogen.cc#newcode1657
src/hydrogen.cc:1657: }
ASSERT if boilerplate and allocation_site->boilerplate are the same

https://codereview.chromium.org/15094018/diff/17001/src/hydrogen.cc#newcode5885
src/hydrogen.cc:5885: Handle<FixedArray> literals(closure->literals(),
isolate());
Literals can be removed.

https://codereview.chromium.org/15094018/diff/17001/src/hydrogen.cc#newcode6018
src/hydrogen.cc:6018: // AllocationSite from the full code gen
why can't you reuse the allocation site generated in full code gen?

https://codereview.chromium.org/15094018/diff/17001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/15094018/diff/17001/src/objects-printer.cc#ne...
src/objects-printer.cc:1122: PrintF(out, " - payload: ");
Why don't we call payload AllocationSite? That would carry more information.

https://codereview.chromium.org/15094018/diff/17001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7462
src/objects.h:7462: DECL_ACCESSORS(payload, Object)
Can we rename payload to something more meaningful? Like transition_data...?

https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7467
src/objects.h:7467: set_payload(Smi::FromInt(GetInitialFastElementsKind()));
You can call SetElementsKindPayload there.

https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7468
src/objects.h:7468: }
declaration order: Typedefs and Enums, Constants (static const data members),
Constructors, Destructor, Methods including static methods,
Data Members (except static const data members)

Moreover can we keep the static methods together?

https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7491
src/objects.h:7491: return !payload()->IsSmi();  // payload is a boilerplate
remove comment or make it clear.

https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7499
src/objects.h:7499: class AllocationSiteInfo: public Struct {
declaration order... I just saw that it is not consistent in our code base but
the google coding style guide suggest to do that. Let's be a good example.

https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7511
src/objects.h:7511: static const int kPayloadOffset = HeapObject::kHeaderSize;
Do you think it makes sense to rename kPayloadOffset to kAllocationSiteOffset? I
do not think that we are going to add more fields to AllocationSiteInfo.

https://codereview.chromium.org/15094018/diff/17001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/15094018/diff/17001/src/runtime.cc#newcode532
src/runtime.cc:532: RUNTIME_FUNCTION(MaybeObject*,
Runtime_CreateArrayLiteralShallow) {
Can we refactor Runtime_CreateArrayLiteral and
Runtime_CreateArrayLiteralShallow. They share quite some similarities.

[mvstanton, 2013-07-02 13:55:11]

Hi Hannes, thanks for the review, I've updated per your comments. Three things
to address:

1) There is new code, giving support to new Array() call sites.
2) ports are not done.
3) I'm doing a perf investigation on kraken, might result in some changes but
they should be local.

https://codereview.chromium.org/15094018/diff/17001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/15094018/diff/17001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:320: AddInstruction(new(zone) HLoadKeyed(literals,
On 2013/06/25 16:35:07, Hannes Payer wrote:
> literals is just used there, you could just use the original code.

Done.

https://codereview.chromium.org/15094018/diff/17001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/15094018/diff/17001/src/flag-definitions.h#ne...
src/flag-definitions.h:270: DEFINE_bool(optimize_constructed_arrays, false,
On 2013/06/25 16:35:07, Hannes Payer wrote:
> Should be true.

Cool, this flag is gone now.

https://codereview.chromium.org/15094018/diff/17001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/15094018/diff/17001/src/hydrogen.cc#newcode5885
src/hydrogen.cc:5885: Handle<FixedArray> literals(closure->literals(),
isolate());
On 2013/06/25 16:35:07, Hannes Payer wrote:
> Literals can be removed.

Done.

https://codereview.chromium.org/15094018/diff/17001/src/hydrogen.cc#newcode6018
src/hydrogen.cc:6018: // AllocationSite from the full code gen
On 2013/06/25 16:35:07, Hannes Payer wrote:
> why can't you reuse the allocation site generated in full code gen?

The reason is that we don't create the AllocationSite at compile time, we create
it when we create the boilerplate object. So we don't have one to look at if we
don't have a boilerplate.

I guess we don't come into this case very much (where we are crankshafting but
apparently we never ran the code even once in full codegen, which would have
created the boilerplate).

https://codereview.chromium.org/15094018/diff/17001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/15094018/diff/17001/src/objects-printer.cc#ne...
src/objects-printer.cc:1122: PrintF(out, " - payload: ");
On 2013/06/25 16:35:07, Hannes Payer wrote:
> Why don't we call payload AllocationSite? That would carry more information.

Done, but the type has to remain object because it can get cleared to Undefined
when type cells are cleared.

https://codereview.chromium.org/15094018/diff/17001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7462
src/objects.h:7462: DECL_ACCESSORS(payload, Object)
On 2013/06/25 16:35:07, Hannes Payer wrote:
> Can we rename payload to something more meaningful? Like transition_data...?

For this one I added a TODO because I'd have to touch a lot. I'd like to do it
in the follow up CL which renames AllocationSiteInfo to be called
AllocationMemento. That will be very simple, even though it touches a lot.

https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7467
src/objects.h:7467: set_payload(Smi::FromInt(GetInitialFastElementsKind()));
On 2013/06/25 16:35:07, Hannes Payer wrote:
> You can call SetElementsKindPayload there.

Done.

https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7468
src/objects.h:7468: }
On 2013/06/25 16:35:07, Hannes Payer wrote:
> declaration order: Typedefs and Enums, Constants (static const data members),
> Constructors, Destructor, Methods including static methods,
> Data Members (except static const data members)
> 
> Moreover can we keep the static methods together?

I think I've arranged it better now, have a look.

https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7491
src/objects.h:7491: return !payload()->IsSmi();  // payload is a boilerplate
On 2013/06/25 16:35:07, Hannes Payer wrote:
> remove comment or make it clear.

Clarified, and made the test stronger: return payload()->IsJSArray().

https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7499
src/objects.h:7499: class AllocationSiteInfo: public Struct {
On 2013/06/25 16:35:07, Hannes Payer wrote:
> declaration order... I just saw that it is not consistent in our code base but
> the google coding style guide suggest to do that. Let's be a good example.

Done.

https://codereview.chromium.org/15094018/diff/17001/src/objects.h#newcode7511
src/objects.h:7511: static const int kPayloadOffset = HeapObject::kHeaderSize;
On 2013/06/25 16:35:07, Hannes Payer wrote:
> Do you think it makes sense to rename kPayloadOffset to kAllocationSiteOffset?
I
> do not think that we are going to add more fields to AllocationSiteInfo.

Done.

https://codereview.chromium.org/15094018/diff/17001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/15094018/diff/17001/src/runtime.cc#newcode532
src/runtime.cc:532: RUNTIME_FUNCTION(MaybeObject*,
Runtime_CreateArrayLiteralShallow) {
On 2013/06/25 16:35:07, Hannes Payer wrote:
> Can we refactor Runtime_CreateArrayLiteral and
> Runtime_CreateArrayLiteralShallow. They share quite some similarities.

thanks, good advice! Done

[mvstanton, 2013-07-03 09:22:33]

Hi Hannes, the perf bug is addressed (see type-info.cc), and the platform ports
are done. thanks!
--Michael

[Hannes Payer (out of office), 2013-07-03 15:26:45]

Another round of comments. I have to look into the platform ports in more
detail.

https://codereview.chromium.org/15094018/diff/52061/src/arm/ic-arm.cc
File src/arm/ic-arm.cc (right):

https://codereview.chromium.org/15094018/diff/52061/src/arm/ic-arm.cc#newcode...
src/arm/ic-arm.cc:1234: FAST_DOUBLE_ELEMENTS);
indent

https://codereview.chromium.org/15094018/diff/52061/src/arm/ic-arm.cc#newcode...
src/arm/ic-arm.cc:1257: FAST_ELEMENTS);
indent

https://codereview.chromium.org/15094018/diff/52061/src/arm/ic-arm.cc#newcode...
src/arm/ic-arm.cc:1388: FAST_DOUBLE_ELEMENTS);
indent

https://codereview.chromium.org/15094018/diff/52061/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/15094018/diff/52061/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:434: HAllocate::Flags flags =
HAllocate::CAN_ALLOCATE_IN_NEW_SPACE;
We could do that right away.

https://codereview.chromium.org/15094018/diff/52061/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:451: /*
This seems to be not finished.

https://codereview.chromium.org/15094018/diff/52061/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/15094018/diff/52061/src/heap.cc#newcode2885
src/heap.cc:2885: }
why do you need that scope?

https://codereview.chromium.org/15094018/diff/52061/src/heap.cc#newcode4497
src/heap.cc:4497: allocation_site);
Can you move AllocateWithAllocationSite into a new line and make the layout
consistent with the following Allocate*WithAllocationsSite calls?

https://codereview.chromium.org/15094018/diff/52061/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/15094018/diff/52061/src/hydrogen.cc#newcode1690
src/hydrogen.cc:1690: HValue* alloc_site) {
I think we should assert that alloc_site is not Handle<Object>::null().

https://codereview.chromium.org/15094018/diff/52061/src/hydrogen.cc#newcode5549
src/hydrogen.cc:5549: isolate());
indent

https://codereview.chromium.org/15094018/diff/52061/src/hydrogen.cc#newcode5561
src/hydrogen.cc:5561: // AllocationSite from the full code gen
Is the uncommented code still needed? What about the comment about copying
useful information?

https://codereview.chromium.org/15094018/diff/52061/src/x64/ic-x64.cc
File src/x64/ic-x64.cc (right):

https://codereview.chromium.org/15094018/diff/52061/src/x64/ic-x64.cc#newcode677
src/x64/ic-x64.cc:677: FAST_DOUBLE_ELEMENTS);
indent

https://codereview.chromium.org/15094018/diff/52061/src/x64/ic-x64.cc#newcode...
src/x64/ic-x64.cc:1622: FAST_DOUBLE_ELEMENTS);
indent

https://codereview.chromium.org/15094018/diff/52061/src/x64/ic-x64.cc#newcode...
src/x64/ic-x64.cc:1647: FAST_ELEMENTS);
indent

[Hannes Payer (out of office), 2013-07-03 15:34:02]

2 more comments

https://codereview.chromium.org/15094018/diff/52061/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/15094018/diff/52061/src/objects.h#newcode7510
src/objects.h:7510: bool site_is_valid() { return
allocation_site()->IsAllocationSite(); }
this is not a real getter.

https://codereview.chromium.org/15094018/diff/52061/src/objects.h#newcode7511
src/objects.h:7511: AllocationSite* allocation_site_casted() {
This is also not a real getter. Moreover can we call it
DependingAllocationSite() or something like that.

[mvstanton, 2013-07-05 07:56:14]

thanks Hannes, getting cleaner now!
--Michael

https://codereview.chromium.org/15094018/diff/52061/src/arm/ic-arm.cc
File src/arm/ic-arm.cc (right):

https://codereview.chromium.org/15094018/diff/52061/src/arm/ic-arm.cc#newcode...
src/arm/ic-arm.cc:1234: FAST_DOUBLE_ELEMENTS);
On 2013/07/03 15:26:45, Hannes Payer wrote:
> indent

Done.

https://codereview.chromium.org/15094018/diff/52061/src/arm/ic-arm.cc#newcode...
src/arm/ic-arm.cc:1257: FAST_ELEMENTS);
On 2013/07/03 15:26:45, Hannes Payer wrote:
> indent

Done.

https://codereview.chromium.org/15094018/diff/52061/src/arm/ic-arm.cc#newcode...
src/arm/ic-arm.cc:1388: FAST_DOUBLE_ELEMENTS);
On 2013/07/03 15:26:45, Hannes Payer wrote:
> indent

Done.

https://codereview.chromium.org/15094018/diff/52061/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/15094018/diff/52061/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:434: HAllocate::Flags flags =
HAllocate::CAN_ALLOCATE_IN_NEW_SPACE;
On 2013/07/03 15:26:45, Hannes Payer wrote:
> We could do that right away.

Done.

https://codereview.chromium.org/15094018/diff/52061/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:451: /*
On 2013/07/03 15:26:45, Hannes Payer wrote:
> This seems to be not finished.

I kept the TODO, and altered it to say come back in the next CL and add an
HAssert instruction (need to add this) to verify that the input cell is really a
Cell.

https://codereview.chromium.org/15094018/diff/52061/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/15094018/diff/52061/src/heap.cc#newcode2885
src/heap.cc:2885: }
On 2013/07/03 15:26:45, Hannes Payer wrote:
> why do you need that scope?

No reason! Removed :).

https://codereview.chromium.org/15094018/diff/52061/src/heap.cc#newcode4497
src/heap.cc:4497: allocation_site);
On 2013/07/03 15:26:45, Hannes Payer wrote:
> Can you move AllocateWithAllocationSite into a new line and make the layout
> consistent with the following Allocate*WithAllocationsSite calls?

I *think* I got what you mean. I changed the indentation for allocation_site to
be 4 spaces in on line 4497. Holler over to me if more is needed, thanks!

https://codereview.chromium.org/15094018/diff/52061/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/15094018/diff/52061/src/hydrogen.cc#newcode1690
src/hydrogen.cc:1690: HValue* alloc_site) {
On 2013/07/03 15:26:45, Hannes Payer wrote:
> I think we should assert that alloc_site is not Handle<Object>::null().

good idea, but I'll just check for a NULL pointer since it's an HValue.

https://codereview.chromium.org/15094018/diff/52061/src/hydrogen.cc#newcode5561
src/hydrogen.cc:5561: // AllocationSite from the full code gen
On 2013/07/03 15:26:45, Hannes Payer wrote:
> Is the uncommented code still needed? What about the comment about copying
> useful information?

Okay, I went ahead and removed this block, I realized it doesn't make sense. The
deal is that if we came in here, we have apparently never created the
boilerplate, so it means we never created the AllocationSite either. For some
reason I had this idea that we did create one before and now had to copy
information out of it. Weird :p.

https://codereview.chromium.org/15094018/diff/52061/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/15094018/diff/52061/src/objects.h#newcode7510
src/objects.h:7510: bool site_is_valid() { return
allocation_site()->IsAllocationSite(); }
On 2013/07/03 15:34:02, Hannes Payer wrote:
> this is not a real getter.

Done.

https://codereview.chromium.org/15094018/diff/52061/src/objects.h#newcode7511
src/objects.h:7511: AllocationSite* allocation_site_casted() {
On 2013/07/03 15:34:02, Hannes Payer wrote:
> This is also not a real getter. Moreover can we call it
> DependingAllocationSite() or something like that.

Done.

https://codereview.chromium.org/15094018/diff/52061/src/x64/ic-x64.cc
File src/x64/ic-x64.cc (right):

https://codereview.chromium.org/15094018/diff/52061/src/x64/ic-x64.cc#newcode677
src/x64/ic-x64.cc:677: FAST_DOUBLE_ELEMENTS);
On 2013/07/03 15:26:45, Hannes Payer wrote:
> indent

Done.

https://codereview.chromium.org/15094018/diff/52061/src/x64/ic-x64.cc#newcode...
src/x64/ic-x64.cc:1622: FAST_DOUBLE_ELEMENTS);
On 2013/07/03 15:26:45, Hannes Payer wrote:
> indent

Done.

https://codereview.chromium.org/15094018/diff/52061/src/x64/ic-x64.cc#newcode...
src/x64/ic-x64.cc:1647: FAST_ELEMENTS);
On 2013/07/03 15:26:45, Hannes Payer wrote:
> indent

Done.

[Hannes Payer (out of office), 2013-07-08 09:03:10]

LGTM! Just minor nits.

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

https://codereview.chromium.org/15094018/diff/71001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:4669: */
It looks like that all comments in that file are of style
// foo
// bar
Let's continue doing that.

https://codereview.chromium.org/15094018/diff/71001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:4679: // sentinels
This comment can be removed.

https://codereview.chromium.org/15094018/diff/71001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:7220: 
Can we add there a comment?

https://codereview.chromium.org/15094018/diff/71001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/15094018/diff/71001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:4715: */
It looks like that all comments in that file are of style
// foo
// bar
Let's continue doing that.

https://codereview.chromium.org/15094018/diff/71001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:4724: // sentinels
This comment can be removed.

https://codereview.chromium.org/15094018/diff/71001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:7819: 
Can we add there a comment?

https://codereview.chromium.org/15094018/diff/71001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/15094018/diff/71001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:3754: */
It looks like that all comments in that file are of style
// foo
// bar
Let's continue doing that.

https://codereview.chromium.org/15094018/diff/71001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:3763: // sentinels
This comment can be removed.

https://codereview.chromium.org/15094018/diff/71001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:6795: 
Can we add there a comment?

[mvstanton, 2013-07-08 09:58:08]

Addressed, thank you very much for the time!

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

https://codereview.chromium.org/15094018/diff/71001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:4669: */
On 2013/07/08 09:03:10, Hannes Payer wrote:
> It looks like that all comments in that file are of style
> // foo
> // bar
> Let's continue doing that.

Done.

https://codereview.chromium.org/15094018/diff/71001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:4679: // sentinels
On 2013/07/08 09:03:10, Hannes Payer wrote:
> This comment can be removed.

Done.

https://codereview.chromium.org/15094018/diff/71001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:7220: 
On 2013/07/08 09:03:10, Hannes Payer wrote:
> Can we add there a comment?

Good idea. I added it to the comment above and closed up the whitespace.

https://codereview.chromium.org/15094018/diff/71001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/15094018/diff/71001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:3754: */
On 2013/07/08 09:03:10, Hannes Payer wrote:
> It looks like that all comments in that file are of style
> // foo
> // bar
> Let's continue doing that.

Done.

https://codereview.chromium.org/15094018/diff/71001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:3763: // sentinels
On 2013/07/08 09:03:10, Hannes Payer wrote:
> This comment can be removed.

Done.

https://codereview.chromium.org/15094018/diff/71001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:6795: 
On 2013/07/08 09:03:10, Hannes Payer wrote:
> Can we add there a comment?

Done (as in arm and ia32, I enhanced the comment above and removed whitespace).

[mvstanton, 2013-07-08 10:02:34]

Committed patchset #15 manually as r15545 (presubmit successful).