Adapt Danno's Track Allocation Info idea to fast literals. When allocating a literal array,

Adapt Danno's Track Allocation Info idea to fast literals. When allocating a literal array,
we store an AllocationSiteInfo object right after the JSArray, with a pointer to the
boilerplate object. Later, if the array transitions we check for the continued existence
of the temporary AllocationSiteInfo object (has no roots). If found, we'll use it to
transition the boilerplate array as well.

Danno's original changeset: https://codereview.chromium.org/10615002/

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

[danno, 2012-12-26 10:32:01]

Here you go...

https://codereview.chromium.org/11663005/diff/2001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/11663005/diff/2001/src/code-stubs.h#newcode390
src/code-stubs.h:390: CLONE_ANY_ELEMENTS_WITH_ALLOCATION_INFO
Add LAST_CLONE_MODE = CLONE_ANY_ELEMENTS_WITH_ALLOCATION_INFO

https://codereview.chromium.org/11663005/diff/2001/src/code-stubs.h#newcode408
src/code-stubs.h:408: ASSERT(mode_ == 0 || mode_ == 1 || mode_ == 2 || mode_ ==
3 ||
While you're here, how about adding a constant
const int kFastCloneModeCount = LAST_CLONE_MODE + 1, then you can ASSERT(mode_
>= 0 && mode_ <= LAST_CLONE_MODE);

https://codereview.chromium.org/11663005/diff/2001/src/code-stubs.h#newcode410
src/code-stubs.h:410: return length_ * 5 +  mode_;
return length_ * kFastCloneModeCount + mode_;

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

https://codereview.chromium.org/11663005/diff/2001/src/ia32/code-stubs-ia32.c...
src/ia32/code-stubs-ia32.cc:314: bool want_allocation_info,
Instead of passing a bool, create a enum in the header (we do this elsewhere)
called something like AllocationInfoMode with TRACK_ALLOCATION_INFO and
DONT_TRACK_ALLOCATION_INFO. This makes it clearer what's going on at a caller's
invocation site. Perhaps just supporting the mode_ without converting to a
separate bool might make the code closer.

https://codereview.chromium.org/11663005/diff/2001/src/ia32/code-stubs-ia32.c...
src/ia32/code-stubs-ia32.cc:344: // type_info_cell);
nit: delete comment

https://codereview.chromium.org/11663005/diff/2001/src/ia32/codegen-ia32.cc
File src/ia32/codegen-ia32.cc (right):

https://codereview.chromium.org/11663005/diff/2001/src/ia32/codegen-ia32.cc#n...
src/ia32/codegen-ia32.cc:394: Label* no_info_available) {
This might be better as a method directly on MacroAssembler like
JumpIfNotInNewSpace.

https://codereview.chromium.org/11663005/diff/2001/src/ia32/codegen-ia32.cc#n...
src/ia32/codegen-ia32.cc:399: //  -- edx    : receiver
This comment isn't correct any more, I think.

https://codereview.chromium.org/11663005/diff/2001/src/ia32/codegen-ia32.cc#n...
src/ia32/codegen-ia32.cc:416: __ cmp(edi,
Immediate(Handle<Map>(masm->isolate()->heap()->
This entire check might be useful as a method directly on MacroAssembler, and I
think you can make it pretty short:

__ lea(edi, Operand(edx, JSArray::kSize + AllocationSiteInfo::kSize));
__ cmp(edi, Operand::StaticVariable(new_space_start)));
__ j(less_than, no_info_available);
__ cmp(edi, Operand::StaticVariable(new_space_allocation_top));
__ j(greater_equal, no_info_available);
__ cmp(MemOperand(edi, 0),
Immediate(Handle<Map>(masm->isolate()->heap()->allocation_site_info_map())));
__ j(not_equal, no_info_available);

To be _really_ clever, we would make sure that every space had 2 * kPointerSize
unused bytes at the end of pages that are allocated in spaces. That way:

__ cmp(MemOperand(edx, JSArray::kSize + AllocationSiteInfo::kSize),
Immediate(Handle<Map>(masm->isolate()->heap()->allocation_site_info_map()));
__ j(not_equal, no_info_available);

would always be safe, and quickly discard non-qualifying candidates. But that's
probably another CL. :-)

https://codereview.chromium.org/11663005/diff/2001/src/ia32/codegen-ia32.cc#n...
src/ia32/codegen-ia32.cc:618: __ bind(&no_allocation_site_info);
I like the idea to not pre-transition any arrays that go double -> object. In
fact, I think we should change all of the logic that transitions or
pre-transitions double->object to instead just used generic stores. But that's
probably another CL.

https://codereview.chromium.org/11663005/diff/2001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/11663005/diff/2001/src/ia32/full-codegen-ia32...
src/ia32/full-codegen-ia32.cc:1683: // useful to pretransition.
This also will track DOUBLE -> FAST_OBJECT, so the comment is a bit misleading,
right?

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

https://codereview.chromium.org/11663005/diff/2001/src/objects-printer.cc#new...
src/objects-printer.cc:1009: JSGlobalPropertyCell* cell =
JSGlobalPropertyCell::cast(payload());
You can remove this case until we support allocation infos for new Array(), this
part of the if is unused for the array literal allocation info tracking.

https://codereview.chromium.org/11663005/diff/2001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11663005/diff/2001/src/objects.cc#newcode10641
src/objects.cc:10641: if (FLAG_use_allocation_site_info &&
GetHeap()->InNewSpace(this)) {
The machinery that checks/gets an AllocationSiteInfo for an Object might be
nicely encapsulated as a static method on the AllocationSiteInfo, either
returning a valid AllocationSizeInfo* or NULL/undefined if there is none.

https://codereview.chromium.org/11663005/diff/2001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11663005/diff/2001/src/objects.h#newcode4167
src/objects.h:4167: 
nit: remove whitespace change

[mvstanton, 2013-01-03 14:40:43]

Hi Danno, I think this is ready for a look. Got the other platforms, addressed
issues.

https://codereview.chromium.org/11663005/diff/2001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/11663005/diff/2001/src/code-stubs.h#newcode390
src/code-stubs.h:390: CLONE_ANY_ELEMENTS_WITH_ALLOCATION_INFO
On 2012/12/26 10:32:01, danno wrote:
> Add LAST_CLONE_MODE = CLONE_ANY_ELEMENTS_WITH_ALLOCATION_INFO

Done.

https://codereview.chromium.org/11663005/diff/2001/src/code-stubs.h#newcode408
src/code-stubs.h:408: ASSERT(mode_ == 0 || mode_ == 1 || mode_ == 2 || mode_ ==
3 ||
On 2012/12/26 10:32:01, danno wrote:
> While you're here, how about adding a constant
> const int kFastCloneModeCount = LAST_CLONE_MODE + 1, then you can ASSERT(mode_
> >= 0 && mode_ <= LAST_CLONE_MODE);

Done.

https://codereview.chromium.org/11663005/diff/2001/src/code-stubs.h#newcode410
src/code-stubs.h:410: return length_ * 5 +  mode_;
On 2012/12/26 10:32:01, danno wrote:
> return length_ * kFastCloneModeCount + mode_;

Done.

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

https://codereview.chromium.org/11663005/diff/2001/src/ia32/code-stubs-ia32.c...
src/ia32/code-stubs-ia32.cc:314: bool want_allocation_info,
On 2012/12/26 10:32:01, danno wrote:
> Instead of passing a bool, create a enum in the header (we do this elsewhere)
> called something like AllocationInfoMode with TRACK_ALLOCATION_INFO and
> DONT_TRACK_ALLOCATION_INFO. This makes it clearer what's going on at a
caller's
> invocation site. Perhaps just supporting the mode_ without converting to a
> separate bool might make the code closer.

Done...I made a separate enum. I'd like to coalesce the two fields which seems
like the right thing to do. But what happens is that one parent function will
make multiple calls to this function with different
FastCloneShallowArrayStub::Modes, building one general stub from several
specific cases. It seemed better to separate the allocationinfomode concern from
the elements/double concern.

https://codereview.chromium.org/11663005/diff/2001/src/ia32/code-stubs-ia32.c...
src/ia32/code-stubs-ia32.cc:344: // type_info_cell);
On 2012/12/26 10:32:01, danno wrote:
> nit: delete comment

Done.

https://codereview.chromium.org/11663005/diff/2001/src/ia32/codegen-ia32.cc
File src/ia32/codegen-ia32.cc (right):

https://codereview.chromium.org/11663005/diff/2001/src/ia32/codegen-ia32.cc#n...
src/ia32/codegen-ia32.cc:394: Label* no_info_available) {
On 2012/12/26 10:32:01, danno wrote:
> This might be better as a method directly on MacroAssembler like
> JumpIfNotInNewSpace.

Done.

https://codereview.chromium.org/11663005/diff/2001/src/ia32/codegen-ia32.cc#n...
src/ia32/codegen-ia32.cc:399: //  -- edx    : receiver
On 2012/12/26 10:32:01, danno wrote:
> This comment isn't correct any more, I think.

I changed it to just talk about edx and edi, the only registers involved.

https://codereview.chromium.org/11663005/diff/2001/src/ia32/codegen-ia32.cc#n...
src/ia32/codegen-ia32.cc:416: __ cmp(edi,
Immediate(Handle<Map>(masm->isolate()->heap()->
On 2012/12/26 10:32:01, danno wrote:
> This entire check might be useful as a method directly on MacroAssembler, and
I
> think you can make it pretty short:
> 
> __ lea(edi, Operand(edx, JSArray::kSize + AllocationSiteInfo::kSize));
> __ cmp(edi, Operand::StaticVariable(new_space_start)));
> __ j(less_than, no_info_available);
> __ cmp(edi, Operand::StaticVariable(new_space_allocation_top));
> __ j(greater_equal, no_info_available);
> __ cmp(MemOperand(edi, 0),
> Immediate(Handle<Map>(masm->isolate()->heap()->allocation_site_info_map())));
> __ j(not_equal, no_info_available);
> 
> To be _really_ clever, we would make sure that every space had 2 *
kPointerSize
> unused bytes at the end of pages that are allocated in spaces. That way:
> 
> __ cmp(MemOperand(edx, JSArray::kSize + AllocationSiteInfo::kSize),
> Immediate(Handle<Map>(masm->isolate()->heap()->allocation_site_info_map()));
> __ j(not_equal, no_info_available);
> 
> would always be safe, and quickly discard non-qualifying candidates. But
that's
> probably another CL. :-)

Added issue http://code.google.com/p/v8/issues/detail?id=2467 to make sure I
remember.

https://codereview.chromium.org/11663005/diff/2001/src/ia32/codegen-ia32.cc#n...
src/ia32/codegen-ia32.cc:618: __ bind(&no_allocation_site_info);
On 2012/12/26 10:32:01, danno wrote:
> I like the idea to not pre-transition any arrays that go double -> object. In
> fact, I think we should change all of the logic that transitions or
> pre-transitions double->object to instead just used generic stores. But that's
> probably another CL.

I'm trying this out on all platforms with that section commented out...

https://codereview.chromium.org/11663005/diff/2001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/11663005/diff/2001/src/ia32/full-codegen-ia32...
src/ia32/full-codegen-ia32.cc:1683: // useful to pretransition.
On 2012/12/26 10:32:01, danno wrote:
> This also will track DOUBLE -> FAST_OBJECT, so the comment is a bit
misleading,
> right?

Done.

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

https://codereview.chromium.org/11663005/diff/2001/src/objects-printer.cc#new...
src/objects-printer.cc:1009: JSGlobalPropertyCell* cell =
JSGlobalPropertyCell::cast(payload());
On 2012/12/26 10:32:01, danno wrote:
> You can remove this case until we support allocation infos for new Array(),
this
> part of the if is unused for the array literal allocation info tracking. 

Done.

https://codereview.chromium.org/11663005/diff/2001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11663005/diff/2001/src/objects.cc#newcode10641
src/objects.cc:10641: if (FLAG_use_allocation_site_info &&
GetHeap()->InNewSpace(this)) {
On 2012/12/26 10:32:01, danno wrote:
> The machinery that checks/gets an AllocationSiteInfo for an Object might be
> nicely encapsulated as a static method on the AllocationSiteInfo, either
> returning a valid AllocationSizeInfo* or NULL/undefined if there is none.

Done.

https://codereview.chromium.org/11663005/diff/2001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11663005/diff/2001/src/objects.h#newcode4167
src/objects.h:4167: 
On 2012/12/26 10:32:01, danno wrote:
> nit: remove whitespace change

Done.

[danno, 2013-01-04 08:50:55]

Getting pretty close... mostly minor stuff.

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

https://codereview.chromium.org/11663005/diff/9016/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:382: FieldMemOperand(r0, allocation_info_start));
nit: one line

https://codereview.chromium.org/11663005/diff/9016/src/arm/macro-assembler-ar...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/11663005/diff/9016/src/arm/macro-assembler-ar...
src/arm/macro-assembler-arm.cc:3890: //  -- r2    : receiver
Pass in receiver_reg and scratch_reg.

https://codereview.chromium.org/11663005/diff/9016/src/arm/macro-assembler-ar...
src/arm/macro-assembler-arm.cc:3906: allocation_site_info_map())));
nit: formatting above, Operand can be on same line as r4 and
allocation_site_info_map should be indented 4 spaces.

https://codereview.chromium.org/11663005/diff/9016/src/arm/macro-assembler-ar...
src/arm/macro-assembler-arm.cc:3908: // Use the j/jmp sequence below for
debugging, but the j(equal) sequence
Keep the sequence below uncommented on this and other platforms, just wrap it in
a if (FLAG_debug_code) {}

https://codereview.chromium.org/11663005/diff/9016/src/arm/macro-assembler-arm.h
File src/arm/macro-assembler-arm.h (right):

https://codereview.chromium.org/11663005/diff/9016/src/arm/macro-assembler-ar...
src/arm/macro-assembler-arm.h:1317: void PerformAllocationSiteInfoCheck(Label*
allocation_info_present);
Pass the two registers that are used into this function like in other functions
in the macro assembler (for example, take a look at InitializeNewString). Then
specify r2 and r4 at the calling site. Same on other platforms, too.

You might want to make the name here and on other platforms mention arrays, too,
since this routine only works for JSArrays. For example,
TestJSArrayForAllocationSiteInfo

https://codereview.chromium.org/11663005/diff/9016/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/11663005/diff/9016/src/code-stubs.h#newcode430
src/code-stubs.h:430: CLONE_ANY_ELEMENTS_WITH_ALLOCATION_INFO,
CLONE_ANY_ELEMENTS_WITH_ALLOCATION_SITE_INFO?

https://codereview.chromium.org/11663005/diff/9016/src/code-stubs.h#newcode437
src/code-stubs.h:437: enum AllocationInfoMode {
AllocationSiteInfoMode?

https://codereview.chromium.org/11663005/diff/9016/src/code-stubs.h#newcode440
src/code-stubs.h:440: };
Soon this will be used for other things, so I think it makes sense to move this
out of the FastCloneShallowArrayStub class. Maybe it belongs in objects.h before
the AllocationSiteInfo class itself. The constants should probably be consistent
with the class name, too, like XXX_ALLOCATION_SITE_INFO.

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

https://codereview.chromium.org/11663005/diff/9016/src/flag-definitions.h#new...
src/flag-definitions.h:212: DEFINE_bool(use_allocation_site_info, true,
how about just "track_allocation_sites"

https://codereview.chromium.org/11663005/diff/9016/src/ia32/codegen-ia32.cc
File src/ia32/codegen-ia32.cc (right):

https://codereview.chromium.org/11663005/diff/9016/src/ia32/codegen-ia32.cc#n...
src/ia32/codegen-ia32.cc:566: // }
Just remove this altogether rather than leave it uncommented.

https://codereview.chromium.org/11663005/diff/9016/src/ia32/macro-assembler-i...
File src/ia32/macro-assembler-ia32.h (right):

https://codereview.chromium.org/11663005/diff/9016/src/ia32/macro-assembler-i...
src/ia32/macro-assembler-ia32.h:867: void PerformAllocationSiteInfoCheck(Label*
allocation_info_present);
See comments in macro-assembler-arm.h.

https://codereview.chromium.org/11663005/diff/9016/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/11663005/diff/9016/src/objects-debug.cc#newco...
src/objects-debug.cc:778: CHECK(IsAllocationSiteInfo());
Verify that the payload is also an Object.

https://codereview.chromium.org/11663005/diff/9016/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11663005/diff/9016/src/objects.cc#newcode7905
src/objects.cc:7905: AllocationSiteInfo*
AllocationSiteInfo::FindAfterJSObject(JSObject* object) {
How about "FindForJSObject?" FindAfterJSObject implies the implementation detail
of how the object is actually found, which the caller doesn't really care about.

https://codereview.chromium.org/11663005/diff/9016/src/objects.cc#newcode7906
src/objects.cc:7906: // Are we in new space and using the allocation site info
feature?
nit: the comment seems a little superfluous, since the following code line does
exactly what it states. Perhaps instead a comment describing under which
circumstances such an object is found, i.e. something like "currently,
AllocationSiteInfo objects are only allocated immediately after JSArrays in
NewSpace, and detecting whether a JSArray has one involves carefully checking
the object immediately after the JSArray if there is one whether it qualifies".

https://codereview.chromium.org/11663005/diff/9016/src/objects.cc#newcode10821
src/objects.cc:10821: MaybeObject*
JSObject::PossiblyTransitionBoilerplate(ElementsKind to_kind) {
Do you want to ASSERT(IsJSArray())? Maybe make the name
PossibleTransitionArrayBoilerplate?

https://codereview.chromium.org/11663005/diff/9016/src/objects.cc#newcode10823
src/objects.cc:10823: // Are we in new space?
nit: comment seems incorrect, the FindAfterJSObject does more.

https://codereview.chromium.org/11663005/diff/9016/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11663005/diff/9016/src/objects.h#newcode6915
src/objects.h:6915: void AllocationSiteInfoPrint(FILE* out);
Maybe use some variant of DECLARE_PRINTER?

https://codereview.chromium.org/11663005/diff/9016/src/objects.h#newcode6918
src/objects.h:6918: void AllocationSiteInfoVerify();
Use DECLARE_VERIFIER instead of explicitly declaring the Verifier yourself.

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

https://codereview.chromium.org/11663005/diff/9016/src/x64/code-stubs-x64.cc#...
src/x64/code-stubs-x64.cc:420: FastCloneShallowArrayStub::AllocationInfoMode
allocation_info_mode = DONT_TRACK_ALLOCATION_INFO;
80 col

https://codereview.chromium.org/11663005/diff/9016/src/x64/codegen-x64.cc
File src/x64/codegen-x64.cc (right):

https://codereview.chromium.org/11663005/diff/9016/src/x64/codegen-x64.cc#new...
src/x64/codegen-x64.cc:414: // }
Remove

https://codereview.chromium.org/11663005/diff/9016/src/x64/macro-assembler-x64.h
File src/x64/macro-assembler-x64.h (right):

https://codereview.chromium.org/11663005/diff/9016/src/x64/macro-assembler-x6...
src/x64/macro-assembler-x64.h:1323: void PerformAllocationSiteInfoCheck(Label*
allocation_info_present);
Pass in registers

https://codereview.chromium.org/11663005/diff/9016/test/mjsunit/array-natives...
File test/mjsunit/array-natives-elements.js (right):

https://codereview.chromium.org/11663005/diff/9016/test/mjsunit/array-natives...
test/mjsunit/array-natives-elements.js:317: }
unintended whitespace change?

[mvstanton, 2013-01-04 12:07:52]

Thanks for the good ideas, I've updated the changeset!

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

https://codereview.chromium.org/11663005/diff/9016/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:382: FieldMemOperand(r0, allocation_info_start));
On 2013/01/04 08:50:55, danno wrote:
> nit: one line

Done.

https://codereview.chromium.org/11663005/diff/9016/src/arm/macro-assembler-ar...
File src/arm/macro-assembler-arm.cc (right):

https://codereview.chromium.org/11663005/diff/9016/src/arm/macro-assembler-ar...
src/arm/macro-assembler-arm.cc:3890: //  -- r2    : receiver
On 2013/01/04 08:50:55, danno wrote:
> Pass in receiver_reg and scratch_reg.

Done.

https://codereview.chromium.org/11663005/diff/9016/src/arm/macro-assembler-ar...
src/arm/macro-assembler-arm.cc:3906: allocation_site_info_map())));
On 2013/01/04 08:50:55, danno wrote:
> nit: formatting above, Operand can be on same line as r4 and
> allocation_site_info_map should be indented 4 spaces.

Done.

https://codereview.chromium.org/11663005/diff/9016/src/arm/macro-assembler-ar...
src/arm/macro-assembler-arm.cc:3908: // Use the j/jmp sequence below for
debugging, but the j(equal) sequence
I decided to remove it because debug_code should be for additional checks,
whereas my int3() was just so a developer can stop in a debugger to examine the
generated code. I will think about doing some debug_code work in the next
checkin, which does more extensive things with the AllocationSiteInfo payload.
(I can check if the payload is a smi or object, for example).

https://codereview.chromium.org/11663005/diff/9016/src/arm/macro-assembler-arm.h
File src/arm/macro-assembler-arm.h (right):

https://codereview.chromium.org/11663005/diff/9016/src/arm/macro-assembler-ar...
src/arm/macro-assembler-arm.h:1317: void PerformAllocationSiteInfoCheck(Label*
allocation_info_present);
On 2013/01/04 08:50:55, danno wrote:
> Pass the two registers that are used into this function like in other
functions
> in the macro assembler (for example, take a look at InitializeNewString). Then
> specify r2 and r4 at the calling site. Same on other platforms, too.
> 
> You might want to make the name here and on other platforms mention arrays,
too,
> since this routine only works for JSArrays. For example,
> TestJSArrayForAllocationSiteInfo

Done.

https://codereview.chromium.org/11663005/diff/9016/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/11663005/diff/9016/src/code-stubs.h#newcode430
src/code-stubs.h:430: CLONE_ANY_ELEMENTS_WITH_ALLOCATION_INFO,
On 2013/01/04 08:50:55, danno wrote:
> CLONE_ANY_ELEMENTS_WITH_ALLOCATION_SITE_INFO?

Done.

https://codereview.chromium.org/11663005/diff/9016/src/code-stubs.h#newcode437
src/code-stubs.h:437: enum AllocationInfoMode {
On 2013/01/04 08:50:55, danno wrote:
> AllocationSiteInfoMode?

Done.

https://codereview.chromium.org/11663005/diff/9016/src/code-stubs.h#newcode440
src/code-stubs.h:440: };
On 2013/01/04 08:50:55, danno wrote:
> Soon this will be used for other things, so I think it makes sense to move
this
> out of the FastCloneShallowArrayStub class. Maybe it belongs in objects.h
before
> the AllocationSiteInfo class itself. The constants should probably be
consistent
> with the class name, too, like XXX_ALLOCATION_SITE_INFO.

Done.

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

https://codereview.chromium.org/11663005/diff/9016/src/flag-definitions.h#new...
src/flag-definitions.h:212: DEFINE_bool(use_allocation_site_info, true,
On 2013/01/04 08:50:55, danno wrote:
> how about just "track_allocation_sites"

Done.

https://codereview.chromium.org/11663005/diff/9016/src/ia32/codegen-ia32.cc
File src/ia32/codegen-ia32.cc (right):

https://codereview.chromium.org/11663005/diff/9016/src/ia32/codegen-ia32.cc#n...
src/ia32/codegen-ia32.cc:566: // }
On 2013/01/04 08:50:55, danno wrote:
> Just remove this altogether rather than leave it uncommented.

Done.

https://codereview.chromium.org/11663005/diff/9016/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11663005/diff/9016/src/objects.cc#newcode7905
src/objects.cc:7905: AllocationSiteInfo*
AllocationSiteInfo::FindAfterJSObject(JSObject* object) {
On 2013/01/04 08:50:55, danno wrote:
> How about "FindForJSObject?" FindAfterJSObject implies the implementation
detail
> of how the object is actually found, which the caller doesn't really care
about.

Done.

https://codereview.chromium.org/11663005/diff/9016/src/objects.cc#newcode7906
src/objects.cc:7906: // Are we in new space and using the allocation site info
feature?
On 2013/01/04 08:50:55, danno wrote:
> nit: the comment seems a little superfluous, since the following code line
does
> exactly what it states. Perhaps instead a comment describing under which
> circumstances such an object is found, i.e. something like "currently,
> AllocationSiteInfo objects are only allocated immediately after JSArrays in
> NewSpace, and detecting whether a JSArray has one involves carefully checking
> the object immediately after the JSArray if there is one whether it
qualifies". 

Way more useful comment, thx!

https://codereview.chromium.org/11663005/diff/9016/src/objects.cc#newcode10821
src/objects.cc:10821: MaybeObject*
JSObject::PossiblyTransitionBoilerplate(ElementsKind to_kind) {
On 2013/01/04 08:50:55, danno wrote:
> Do you want to ASSERT(IsJSArray())? Maybe make the name
> PossibleTransitionArrayBoilerplate?

Done.

https://codereview.chromium.org/11663005/diff/9016/src/objects.cc#newcode10823
src/objects.cc:10823: // Are we in new space?
On 2013/01/04 08:50:55, danno wrote:
> nit: comment seems incorrect, the FindAfterJSObject does more.

Done.

https://codereview.chromium.org/11663005/diff/9016/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11663005/diff/9016/src/objects.h#newcode6915
src/objects.h:6915: void AllocationSiteInfoPrint(FILE* out);
On 2013/01/04 08:50:55, danno wrote:
> Maybe use some variant of DECLARE_PRINTER?

Done.

https://codereview.chromium.org/11663005/diff/9016/src/objects.h#newcode6918
src/objects.h:6918: void AllocationSiteInfoVerify();
On 2013/01/04 08:50:55, danno wrote:
> Use DECLARE_VERIFIER instead of explicitly declaring the Verifier yourself.

Done.

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

https://codereview.chromium.org/11663005/diff/9016/src/x64/code-stubs-x64.cc#...
src/x64/code-stubs-x64.cc:420: FastCloneShallowArrayStub::AllocationInfoMode
allocation_info_mode = DONT_TRACK_ALLOCATION_INFO;
On 2013/01/04 08:50:55, danno wrote:
> 80 col

Done.

https://codereview.chromium.org/11663005/diff/9016/test/mjsunit/array-natives...
File test/mjsunit/array-natives-elements.js (right):

https://codereview.chromium.org/11663005/diff/9016/test/mjsunit/array-natives...
test/mjsunit/array-natives-elements.js:317: }
On 2013/01/04 08:50:55, danno wrote:
> unintended whitespace change?

Done.

[danno, 2013-01-07 08:03:48]

LGTM with comments addressed and a last final check that perf doesn't regress.

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

https://codereview.chromium.org/11663005/diff/16004/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:347: AllocationSiteInfoMode allocation_site_info_mode,
Do we really need a new enum type for this (yet?). It seems that you can discern
whether an AllocationSiteInfo is necessary directly from mode, and since we
don't yet have allocation routines that would only need AllocationSiteInfoMode
without some other context, maybe it makes sense to postpone adding the enum
until that time?

https://codereview.chromium.org/11663005/diff/16004/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/11663005/diff/16004/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:3073: allocation_site_info_map())));
nit: indentation (4 spaces)

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

https://codereview.chromium.org/11663005/diff/16004/src/objects-printer.cc#ne...
src/objects-printer.cc:1009: PrintF(out, "Array literal boilerplate ");
nit: remove boilerplate, since we will change this to support non-boilerplate
transitions and don't want to forget to change this.

https://codereview.chromium.org/11663005/diff/16004/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11663005/diff/16004/src/objects.cc#newcode7916
src/objects.cc:7916: Map** possible_allocation_map =
reinterpret_cast<Map**>(ptr_end);
nit: perhaps possible_allocation_info_map?

https://codereview.chromium.org/11663005/diff/16004/src/objects.cc#newcode10835
src/objects.cc:10835: if (ret->IsFailure()) return ret;
nit: statement above is unnecessary, since you will fall through to return
anyway.

[mvstanton, 2013-01-07 09:28:41]

Thanks Danno, last changes, and performance job kicked off, let's review that
briefly in a couple hours.

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

https://codereview.chromium.org/11663005/diff/16004/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:347: AllocationSiteInfoMode allocation_site_info_mode,
On 2013/01/07 08:03:48, danno wrote:
> Do we really need a new enum type for this (yet?). It seems that you can
discern
> whether an AllocationSiteInfo is necessary directly from mode, and since we
> don't yet have allocation routines that would only need AllocationSiteInfoMode
> without some other context, maybe it makes sense to postpone adding the enum
> until that time?

As we discussed, I'll keep it because this helper method is a bit quirky,
asserting that the mode flag is just a specific subset of the possible values.
That has the effect of splitting off the allocation site info decision into
another flag. I'd also like to keep the decision about doing or not doing
certain pretransitions (such as the problematic DOUBLE->FAST) upstream...it is
handled at a higher level.

https://codereview.chromium.org/11663005/diff/16004/src/ia32/macro-assembler-...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/11663005/diff/16004/src/ia32/macro-assembler-...
src/ia32/macro-assembler-ia32.cc:3073: allocation_site_info_map())));
On 2013/01/07 08:03:48, danno wrote:
> nit: indentation (4 spaces)

Done.

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

https://codereview.chromium.org/11663005/diff/16004/src/objects-printer.cc#ne...
src/objects-printer.cc:1009: PrintF(out, "Array literal boilerplate ");
On 2013/01/07 08:03:48, danno wrote:
> nit: remove boilerplate, since we will change this to support non-boilerplate
> transitions and don't want to forget to change this.

Done.

https://codereview.chromium.org/11663005/diff/16004/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11663005/diff/16004/src/objects.cc#newcode7916
src/objects.cc:7916: Map** possible_allocation_map =
reinterpret_cast<Map**>(ptr_end);
On 2013/01/07 08:03:48, danno wrote:
> nit: perhaps possible_allocation_info_map?

I went ahead and dogmatically named it possible_allocation_site_info_map :).

https://codereview.chromium.org/11663005/diff/16004/src/objects.cc#newcode10835
src/objects.cc:10835: if (ret->IsFailure()) return ret;
On 2013/01/07 08:03:48, danno wrote:
> nit: statement above is unnecessary, since you will fall through to return
> anyway.

Done.