Additional work to get array literal allocation tracking working, even with --always-opt

Additional work to get array literal allocation tracking working, even with --always-opt

BUG=

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

[mvstanton, 2013-01-10 15:48:51]

Here is follow up work for tracking allocation site info on literal arrays. 

There are a few TODOs meant to identify the next step. One TODO may need to be
addressed now, can you look at lithium-codegen-ia32.cc, line 5340? The
FastCloneShallowArrayStub::Mode flag is a bit awkward.

[mvstanton, 2013-01-10 17:02:41]

Hi guys, Toon or Michael will do the review, thx!

[danno, 2013-01-10 22:58:59]

Here's a first round. Sorry, couldn't resist... ;-)

https://codereview.chromium.org/11817017/diff/6001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

https://codereview.chromium.org/11817017/diff/6001/src/arm/codegen-arm.cc#new...
src/arm/codegen-arm.cc:147: MacroAssembler* masm, Label* fail) {
nit: here and elsewhere it might be clearer if you called the label
found_allocation_site_info rather than fail.

https://codereview.chromium.org/11817017/diff/6001/src/arm/lithium-codegen-ar...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/11817017/diff/6001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:5476: // all the time?
Be careful (here and on other platforms). Originally when I wrote this code, I
only had the CLONE_ANY_ELEMENTS version. When it landed in Chromium, I tanked
performance in the page cycler performance bots and had to make sure that the
CLONE_ELEMENTS and CLONE_DOUBLE_ELEMENTS were used whenever possible. May it's
time to separate FastCloneShallowArrayStub::Mode and the flag to create site
info always into to fields, and generate 6 versions of the copy stub,
CLONE_DOUBLE, CLONE_ELEMENTS, CLONE_ANY in two versions, one with
AllocationInfo, one without.

https://codereview.chromium.org/11817017/diff/6001/src/arm/lithium-codegen-arm.h
File src/arm/lithium-codegen-arm.h (right):

https://codereview.chromium.org/11817017/diff/6001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.h:369: bool create_allocation_site_info = false);
Use the AllocationSiteInfoMode enum here instead of a boolean.

https://codereview.chromium.org/11817017/diff/6001/src/codegen.h
File src/codegen.h (right):

https://codereview.chromium.org/11817017/diff/6001/src/codegen.h#newcode101
src/codegen.h:101: Label* fail = NULL);
Maybe it makes sense to not provide a default value for fail (which should be
called allocation_site_info_found), it forces callees to consciously decide
(forgetting the parameter could cause subtle bugs).

https://codereview.chromium.org/11817017/diff/6001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/11817017/diff/6001/src/heap.cc#newcode4398
src/heap.cc:4398: if (track_origin && InNewSpace(clone)) {
This is a bit dicey. Although I don't see a reason that the two allocations (the
object above and the AllocationSiteInfo) won't be contiguous, it's a bit
fragile. If track_origin is false, you should call AllocateRaw as before. If
it's true, you should check object_size against:

  Heap* heap = isolate()->heap();
  const int kMinFreeNewSpaceAfterGC = heap->InitialSemiSpaceSize() * 3/4;

and if it's smaller, call AllocateInNewSpace for both objects together, it
_must_ succeed in this case. In the case that object_size is greater than
kMinFreeNewSpaceAfterGC, don't allocate the AllocationSiteInfo at all. 

Also note that in this code you are allocation AllocationSiteInfos for all
objects you allocate, not just JSArrays. Maybe you want add a predicate
CanTrackAllocationSite to Map that returns "true" for JSArray maps, and then
change the track_origin predicate above to contain "&&
source->map()->CanTrackAllocationSite()

https://codereview.chromium.org/11817017/diff/6001/src/heap.cc#newcode4429
src/heap.cc:4429: // TODO(mvstanton): I don't understand this payload? the
original object?
Yes. And as discussed today, this is a bit wrong right now, it kind of assumes
that source is a boilerplate array because this code path gated by track_origin
is only used to copy boilerplate arrays (see my code above to limit tracking to
just JSArrays).

When you copy deep boilerplates, you will have to pass in the AllocationSite to
use rather than the array itself.

https://codereview.chromium.org/11817017/diff/6001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/11817017/diff/6001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4992: bool create_allocation_site_info = false)
Use AllocationSiteInfoMode.

https://codereview.chromium.org/11817017/diff/6001/src/ia32/lithium-codegen-i...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/11817017/diff/6001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:5430: if (create_allocation_site_info) {
Here and other platforms, only do this if and "create_allocation_site_info &&
object->map()->CanTrackAllocationInfo()";

https://codereview.chromium.org/11817017/diff/6001/src/mips/stub-cache-mips.cc
File src/mips/stub-cache-mips.cc (right):

https://codereview.chromium.org/11817017/diff/6001/src/mips/stub-cache-mips.c...
src/mips/stub-cache-mips.cc:3410: a3, Operand(receiver_maps->at(i)));
Stray whitespace change?

https://codereview.chromium.org/11817017/diff/6001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11817017/diff/6001/src/objects.cc#newcode10423
src/objects.cc:10423: if (info->payload()->IsJSArray()) {
You might not need this additional checks yet if you avoid creating the
AllocationSiteInfos for non array types for now.

https://codereview.chromium.org/11817017/diff/6001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/11817017/diff/6001/src/runtime.cc#newcode672
src/runtime.cc:672: TRACK_ALLOCATION_SITE_INFO
nit: ? goes on following line. i.e.
AllocationSiteInfoMode mode = FLAG_track_allocation_sites
    ? TRACK_ALLOCATION_SITE_INFO
    : DONT_TRACK_ALLOCATION_SITE_INFO;

[mvstanton, 2013-01-11 13:43:01]

Thanks for the feedback Danno, happy you couldn't resist a look :). Let me ask
Toon if he is free...

https://codereview.chromium.org/11817017/diff/6001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):

https://codereview.chromium.org/11817017/diff/6001/src/arm/codegen-arm.cc#new...
src/arm/codegen-arm.cc:147: MacroAssembler* masm, Label* fail) {
On 2013/01/10 22:58:59, danno wrote:
> nit: here and elsewhere it might be clearer if you called the label
> found_allocation_site_info rather than fail.

Done.

https://codereview.chromium.org/11817017/diff/6001/src/arm/lithium-codegen-ar...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/11817017/diff/6001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:5476: // all the time?
On 2013/01/10 22:58:59, danno wrote:
> Be careful (here and on other platforms). Originally when I wrote this code, I
> only had the CLONE_ANY_ELEMENTS version. When it landed in Chromium, I tanked
> performance in the page cycler performance bots and had to make sure that the
> CLONE_ELEMENTS and CLONE_DOUBLE_ELEMENTS were used whenever possible. May it's
> time to separate FastCloneShallowArrayStub::Mode and the flag to create site
> info always into to fields, and generate 6 versions of the copy stub,
> CLONE_DOUBLE, CLONE_ELEMENTS, CLONE_ANY in two versions, one with
> AllocationInfo, one without.

Done.

https://codereview.chromium.org/11817017/diff/6001/src/arm/lithium-codegen-arm.h
File src/arm/lithium-codegen-arm.h (right):

https://codereview.chromium.org/11817017/diff/6001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.h:369: bool create_allocation_site_info = false);
On 2013/01/10 22:58:59, danno wrote:
> Use the AllocationSiteInfoMode enum here instead of a boolean.

Done.

https://codereview.chromium.org/11817017/diff/6001/src/codegen.h
File src/codegen.h (right):

https://codereview.chromium.org/11817017/diff/6001/src/codegen.h#newcode101
src/codegen.h:101: Label* fail = NULL);
On 2013/01/10 22:58:59, danno wrote:
> Maybe it makes sense to not provide a default value for fail (which should be
> called allocation_site_info_found), it forces callees to consciously decide
> (forgetting the parameter could cause subtle bugs).

Done.

https://codereview.chromium.org/11817017/diff/6001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/11817017/diff/6001/src/heap.cc#newcode4398
src/heap.cc:4398: if (track_origin && InNewSpace(clone)) {
On 2013/01/10 22:58:59, danno wrote:
> This is a bit dicey. Although I don't see a reason that the two allocations
(the
> object above and the AllocationSiteInfo) won't be contiguous, it's a bit
> fragile. If track_origin is false, you should call AllocateRaw as before. If
> it's true, you should check object_size against:
> 
>   Heap* heap = isolate()->heap();
>   const int kMinFreeNewSpaceAfterGC = heap->InitialSemiSpaceSize() * 3/4;
> 
> and if it's smaller, call AllocateInNewSpace for both objects together, it
> _must_ succeed in this case. In the case that object_size is greater than
> kMinFreeNewSpaceAfterGC, don't allocate the AllocationSiteInfo at all. 
> 
> Also note that in this code you are allocation AllocationSiteInfos for all
> objects you allocate, not just JSArrays. Maybe you want add a predicate
> CanTrackAllocationSite to Map that returns "true" for JSArray maps, and then
> change the track_origin predicate above to contain "&&
> source->map()->CanTrackAllocationSite() 

Thanks, good advice. I actually removed the FLAG_track_allocation_sites, as it
seems to be everywhere. I figure that if someone has asked for the feature to be
on as a parameter to the function call, like here, then any flag checking is
upstream.

https://codereview.chromium.org/11817017/diff/6001/src/heap.cc#newcode4429
src/heap.cc:4429: // TODO(mvstanton): I don't understand this payload? the
original object?
On 2013/01/10 22:58:59, danno wrote:
> Yes. And as discussed today, this is a bit wrong right now, it kind of assumes
> that source is a boilerplate array because this code path gated by
track_origin
> is only used to copy boilerplate arrays (see my code above to limit tracking
to
> just JSArrays).
> 
> When you copy deep boilerplates, you will have to pass in the AllocationSite
to
> use rather than the array itself.

Right. For the moment, the boilerplate is exactly the right payload. Later, when
we move to AllocationMomentos, it will live in the AllocationSite object
instead. For this checkin I just remove the TODO.

https://codereview.chromium.org/11817017/diff/6001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/11817017/diff/6001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4992: bool create_allocation_site_info = false)
On 2013/01/10 22:58:59, danno wrote:
> Use AllocationSiteInfoMode.

Done.

https://codereview.chromium.org/11817017/diff/6001/src/ia32/lithium-codegen-i...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/11817017/diff/6001/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:5430: if (create_allocation_site_info) {
On 2013/01/10 22:58:59, danno wrote:
> Here and other platforms, only do this if and "create_allocation_site_info &&
> object->map()->CanTrackAllocationInfo()";

Done.

https://codereview.chromium.org/11817017/diff/6001/src/mips/stub-cache-mips.cc
File src/mips/stub-cache-mips.cc (right):

https://codereview.chromium.org/11817017/diff/6001/src/mips/stub-cache-mips.c...
src/mips/stub-cache-mips.cc:3410: a3, Operand(receiver_maps->at(i)));
On 2013/01/10 22:58:59, danno wrote:
> Stray whitespace change?

Done.

https://codereview.chromium.org/11817017/diff/6001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11817017/diff/6001/src/objects.cc#newcode10423
src/objects.cc:10423: if (info->payload()->IsJSArray()) {
On 2013/01/10 22:58:59, danno wrote:
> You might not need this additional checks yet if you avoid creating the
> AllocationSiteInfos for non array types for now.

Ah, this if is a relic (or forebearer?) of the changes to follow, where we cased
on whether the payload was an array or a globalproperty object to distinguish
between the literal and constructor cases. I guess I can just assert the
condition here. Later, in fact it will change again to be an AllocationSite
object.

https://codereview.chromium.org/11817017/diff/6001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/11817017/diff/6001/src/runtime.cc#newcode672
src/runtime.cc:672: TRACK_ALLOCATION_SITE_INFO
On 2013/01/10 22:58:59, danno wrote:
> nit: ? goes on following line. i.e.
> AllocationSiteInfoMode mode = FLAG_track_allocation_sites
>     ? TRACK_ALLOCATION_SITE_INFO
>     : DONT_TRACK_ALLOCATION_SITE_INFO;

Done.

[danno, 2013-01-11 16:14:40]

a few nits, mostly pedantic. I'll let Toon finish this one.

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

https://codereview.chromium.org/11817017/diff/18001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:364: size += tracking_on ? AllocationSiteInfo::kSize +
elements_size
nit: "? AllocationSiteInfo::kSize + elements_size" on next line

https://codereview.chromium.org/11817017/diff/18001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:432: mode = tracking_on ?
COPY_ON_WRITE_ELEMENTS_WITH_ALLOCATION_SITE_INFO
nit: ? on next line

https://codereview.chromium.org/11817017/diff/18001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:442: mode = tracking_on ?
CLONE_ELEMENTS_WITH_ALLOCATION_SITE_INFO
nit: ? on next line

https://codereview.chromium.org/11817017/diff/18001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:450: mode = tracking_on ?
CLONE_DOUBLE_ELEMENTS_WITH_ALLOCATION_SITE_INFO
nit: ? on next line

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

https://codereview.chromium.org/11817017/diff/18001/src/code-stubs.h#newcode448
src/code-stubs.h:448: static inline bool TrackAllocationSiteInfo(Mode mode) {
nit: "Track" is a command, but you use it a query. How about
ShouldTrackAllocationSiteInfo?

https://codereview.chromium.org/11817017/diff/18001/src/codegen.h
File src/codegen.h (right):

https://codereview.chromium.org/11817017/diff/18001/src/codegen.h#newcode101
src/codegen.h:101: static void
GenerateMapChangeElementsTransition(MacroAssembler* masm,
nit: all parameters need to be aligned in declaration according to style guide,
so param above needs to go on next line and line up with Label at the 4-char
indent.

[mvstanton, 2013-01-11 16:56:06]

Toon did a review, I typed the comments. jeesh.

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

https://codereview.chromium.org/11817017/diff/18001/src/code-stubs.cc#newcode599
src/code-stubs.cc:599: // transition.
Better, pass a parameter to the method indicating track/don'ttrack
(AllocationSiteInfoMode). And in as many places as possible, discover this with
a helper method that hides:
1) the flag
2) to, from elements kind
3) other dynamic cases...e.tc.

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

https://codereview.chromium.org/11817017/diff/18001/src/code-stubs.h#newcode427
src/code-stubs.h:427: CLONE_ELEMENTS_WITH_ALLOCATION_SITE_INFO,
Instead, use a bitfield that combines the AllocationSiteInfoMode with the
FastCloneShallowArrayStub::Mode.

https://codereview.chromium.org/11817017/diff/18001/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

https://codereview.chromium.org/11817017/diff/18001/src/ia32/ic-ia32.cc#newco...
src/ia32/ic-ia32.cc:849:
ElementsTransitionGenerator::GenerateMapChangeElementsTransition(masm, slow);
Rename this parameter to handle_transition_in_runtime.

https://codereview.chromium.org/11817017/diff/18001/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/11817017/diff/18001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5344: :
FastCloneShallowArrayStub::CLONE_ELEMENTS;
Please put this in hydrogen-instructions just like what was done for
HFastLiteral (in fact move the key up to the base class HMaterializedLiteral).
AND reverse this logic anyway.

https://codereview.chromium.org/11817017/diff/18001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11817017/diff/18001/src/objects.cc#newcode10438
src/objects.cc:10438: ElementsKindToString(to_kind));
spaces between 8*1024.
also AllocationSiteInfo->Print().

[mvstanton, 2013-01-15 15:23:04]

Hi Toon, thanks for the review Friday. I've responded. We imagined some
functions that centralized policy around AllocationSiteInfo, and these are the
GetMode(...) functions defined on that class, let me know your thoughts on that.

Uh..I re-based and so now if you try to do a diff on just the later changes you
get all this rebase crud...oops.

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

https://codereview.chromium.org/11817017/diff/18001/src/code-stubs.cc#newcode599
src/code-stubs.cc:599: // transition.
On 2013/01/11 16:56:06, mvstanton wrote:
> Better, pass a parameter to the method indicating track/don'ttrack
> (AllocationSiteInfoMode). And in as many places as possible, discover this
with
> a helper method that hides:
> 1) the flag
> 2) to, from elements kind
> 3) other dynamic cases...e.tc.

I have added these methods, such that the on/off flag is controlled from a
central point, as well as the "business logic" of deciding whether a particular
(from,to) transition situation should be tracked or not.

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

https://codereview.chromium.org/11817017/diff/18001/src/code-stubs.h#newcode427
src/code-stubs.h:427: CLONE_ELEMENTS_WITH_ALLOCATION_SITE_INFO,
On 2013/01/11 16:56:06, mvstanton wrote:
> Instead, use a bitfield that combines the AllocationSiteInfoMode with the
> FastCloneShallowArrayStub::Mode.

Done.

https://codereview.chromium.org/11817017/diff/18001/src/code-stubs.h#newcode448
src/code-stubs.h:448: static inline bool TrackAllocationSiteInfo(Mode mode) {
On 2013/01/11 16:14:40, danno wrote:
> nit: "Track" is a command, but you use it a query. How about
> ShouldTrackAllocationSiteInfo?

Good point, thanks. I took Toon's advice and ended up dismantling this section.

https://codereview.chromium.org/11817017/diff/18001/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

https://codereview.chromium.org/11817017/diff/18001/src/ia32/ic-ia32.cc#newco...
src/ia32/ic-ia32.cc:849:
ElementsTransitionGenerator::GenerateMapChangeElementsTransition(masm, slow);
On 2013/01/11 16:56:06, mvstanton wrote:
> Rename this parameter to handle_transition_in_runtime.

On reflection I think "slow" as in slow path and fast path is a pretty
established part of the vocabulary. Okay for me to let this one remain as is?
(just gimme a yes/no, no need for explainin').

https://codereview.chromium.org/11817017/diff/18001/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/11817017/diff/18001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5344: :
FastCloneShallowArrayStub::CLONE_ELEMENTS;
On 2013/01/11 16:56:06, mvstanton wrote:
> Please put this in hydrogen-instructions just like what was done for
> HFastLiteral (in fact move the key up to the base class HMaterializedLiteral).
> AND reverse this logic anyway.
> 

By stripping the allocation_site_info flag from the
FastCloneShallowArrayStub::Mode enumeration, I was able to restore this code to
what it was before. allocation_site_info_mode is determined upstream now in
hydrogen.

https://codereview.chromium.org/11817017/diff/18001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11817017/diff/18001/src/objects.cc#newcode10438
src/objects.cc:10438: ElementsKindToString(to_kind));
On 2013/01/11 16:56:06, mvstanton wrote:
> spaces between 8*1024.
> also AllocationSiteInfo->Print().

Got the spaces. On reflection, I'd rather not use AllocationSiteInfo->Print()
here. In the future checkins, Print() will be developed, because
AllocationSiteInfo will become more complex (right now it's just a pointer to a
boilerplate). Also, Print() wouldn't be super helpful because I want to print
out a change of state here...a from->to transition. At the most, a Print()
method on the AllocationSiteInfo could inquire about the elements kind of the
boilerplate and print that...it wouldn't know about the transition.

But I will keep in mind the direction for the next checkins: try to centralize
printing stuff on the AllocationSiteInfo object. Will that be okay?

[Toon Verwaest, 2013-01-16 10:53:41]

Some additional nits, mostly formatting.

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

https://codereview.chromium.org/11817017/diff/30002/src/code-stubs.h#newcode433
src/code-stubs.h:433: static const int kFastCloneModeCount = LAST_CLONE_MODE +
1;
Keep empty line between these lines.

https://codereview.chromium.org/11817017/diff/30002/src/code-stubs.h#newcode453
src/code-stubs.h:453: BitField<AllocationSiteInfoMode, 8, 8> {};
You only need 1 bit for this field.

https://codereview.chromium.org/11817017/diff/30002/src/codegen.h
File src/codegen.h (right):

https://codereview.chromium.org/11817017/diff/30002/src/codegen.h#newcode99
src/codegen.h:99: // allocation_site_info_found may be null, if mode is
If |mode| is set to DONT_TRACK_ALLOCATION_SITE_INFO, |allocation_site_info| may
be NULL.

https://codereview.chromium.org/11817017/diff/30002/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/11817017/diff/30002/src/hydrogen.cc#newcode5330
src/hydrogen.cc:5330: // elements kind is the initial elements kind.
This comment seems to belong above ::GetMode(ElementsKind) rather than here;
given that it describes the return value of GetMode.

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

https://codereview.chromium.org/11817017/diff/30002/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:343: size += tracking_on ? AllocationSiteInfo::kSize
+ elements_size
I prefer the original formatting for this piece of code.

https://codereview.chromium.org/11817017/diff/30002/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/11817017/diff/30002/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5335: FastCloneShallowArrayStub stub(mode,
DONT_TRACK_ALLOCATION_SITE_INFO,
Without _INFO it probably fits on one line; but this should normally be
formatted as

FastCloneShallowArrayStub stub(
    mode, DONT_TRACK_ALLOCATION_SITE_INFO, length);

https://codereview.chromium.org/11817017/diff/30002/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5392: if (create_allocation_site_info) {
I think it's cleaner to just merge both if branches into one.

https://codereview.chromium.org/11817017/diff/30002/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/11817017/diff/30002/src/ic.cc#newcode1856
src/ic.cc:1856: // TODO(mvstanton): we should set the transitioned map to always
be
Should probably clarify that we are talking about pessimistic elements
transitions.

https://codereview.chromium.org/11817017/diff/30002/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11817017/diff/30002/src/objects.cc#newcode7520
src/objects.cc:7520: if (FLAG_track_allocation_sites) {
FLAG_track_allocation_sites && IsFastSmi...

https://codereview.chromium.org/11817017/diff/30002/src/objects.cc#newcode7533
src/objects.cc:7533: if (IsFastSmiElementsKind(from) &&
(IsFastObjectElementsKind(to) ||
Try to put statements between (..) on one line by breaking after &&. Also join
FLAG_track_allocation_sites with && like above.

https://codereview.chromium.org/11817017/diff/30002/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11817017/diff/30002/src/objects.h#newcode6914
src/objects.h:6914: 
Trim empty lines down to 2.

https://codereview.chromium.org/11817017/diff/30002/src/objects.h#newcode6928
src/objects.h:6928: static AllocationSiteInfoMode GetMode();
Just use the flag if no other arguments are required.

[mvstanton, 2013-01-16 13:01:56]

Thanks Toon, updated. You might want to check if the new asserts around
code-stubs.h:453 and the bitfields are okay. Otherwise formatting fixes.

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

https://codereview.chromium.org/11817017/diff/30002/src/code-stubs.h#newcode433
src/code-stubs.h:433: static const int kFastCloneModeCount = LAST_CLONE_MODE +
1;
On 2013/01/16 10:53:42, Toon Verwaest wrote:
> Keep empty line between these lines.

Done.

https://codereview.chromium.org/11817017/diff/30002/src/code-stubs.h#newcode453
src/code-stubs.h:453: BitField<AllocationSiteInfoMode, 8, 8> {};
On 2013/01/16 10:53:42, Toon Verwaest wrote:
> You only need 1 bit for this field.

Fixed, and added a static assert to support it.

https://codereview.chromium.org/11817017/diff/30002/src/codegen.h
File src/codegen.h (right):

https://codereview.chromium.org/11817017/diff/30002/src/codegen.h#newcode99
src/codegen.h:99: // allocation_site_info_found may be null, if mode is
On 2013/01/16 10:53:42, Toon Verwaest wrote:
> If |mode| is set to DONT_TRACK_ALLOCATION_SITE_INFO, |allocation_site_info|
may
> be NULL.

Done.

https://codereview.chromium.org/11817017/diff/30002/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/11817017/diff/30002/src/hydrogen.cc#newcode5330
src/hydrogen.cc:5330: // elements kind is the initial elements kind.
On 2013/01/16 10:53:42, Toon Verwaest wrote:
> This comment seems to belong above ::GetMode(ElementsKind) rather than here;
> given that it describes the return value of GetMode.

Yep, indeed!

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

https://codereview.chromium.org/11817017/diff/30002/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:343: size += tracking_on ? AllocationSiteInfo::kSize
+ elements_size
On 2013/01/16 10:53:42, Toon Verwaest wrote:
> I prefer the original formatting for this piece of code.

Done.

https://codereview.chromium.org/11817017/diff/30002/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/11817017/diff/30002/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5335: FastCloneShallowArrayStub stub(mode,
DONT_TRACK_ALLOCATION_SITE_INFO,
On 2013/01/16 10:53:42, Toon Verwaest wrote:
> Without _INFO it probably fits on one line; but this should normally be
> formatted as
> 
> FastCloneShallowArrayStub stub(
>     mode, DONT_TRACK_ALLOCATION_SITE_INFO, length);

thx, now it does fit on one line.

https://codereview.chromium.org/11817017/diff/30002/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5392: if (create_allocation_site_info) {
On 2013/01/16 10:53:42, Toon Verwaest wrote:
> I think it's cleaner to just merge both if branches into one.

big time, thx.

https://codereview.chromium.org/11817017/diff/30002/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/11817017/diff/30002/src/ic.cc#newcode1856
src/ic.cc:1856: // TODO(mvstanton): we should set the transitioned map to always
be
On 2013/01/16 10:53:42, Toon Verwaest wrote:
> Should probably clarify that we are talking about pessimistic elements
> transitions.

Done.

https://codereview.chromium.org/11817017/diff/30002/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11817017/diff/30002/src/objects.cc#newcode7520
src/objects.cc:7520: if (FLAG_track_allocation_sites) {
On 2013/01/16 10:53:42, Toon Verwaest wrote:
> FLAG_track_allocation_sites && IsFastSmi...

Done.

https://codereview.chromium.org/11817017/diff/30002/src/objects.cc#newcode7533
src/objects.cc:7533: if (IsFastSmiElementsKind(from) &&
(IsFastObjectElementsKind(to) ||
On 2013/01/16 10:53:42, Toon Verwaest wrote:
> Try to put statements between (..) on one line by breaking after &&. Also join
> FLAG_track_allocation_sites with && like above.

Done.

https://codereview.chromium.org/11817017/diff/30002/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11817017/diff/30002/src/objects.h#newcode6914
src/objects.h:6914: 
On 2013/01/16 10:53:42, Toon Verwaest wrote:
> Trim empty lines down to 2.

Done.

https://codereview.chromium.org/11817017/diff/30002/src/objects.h#newcode6928
src/objects.h:6928: static AllocationSiteInfoMode GetMode();
On 2013/01/16 10:53:42, Toon Verwaest wrote:
> Just use the flag if no other arguments are required.

Done.

[Toon Verwaest, 2013-01-16 13:35:55]

LGTM with some last, mostly style-related, nits.

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

https://codereview.chromium.org/11817017/diff/39004/src/code-stubs.h#newcode454
src/code-stubs.h:454: class LengthBits: public BitField<int, 5, 5> {};
You only need 4 bits here.

https://codereview.chromium.org/11817017/diff/39004/src/code-stubs.h#newcode457
src/code-stubs.h:457: STATIC_ASSERT(kMaximumClonedLength <= 8);
With 4 bits, this field supports < 16.

https://codereview.chromium.org/11817017/diff/39004/src/codegen.h
File src/codegen.h (right):

https://codereview.chromium.org/11817017/diff/39004/src/codegen.h#newcode99
src/codegen.h:99: // if |mode| is set to DONT_TRACK_ALLOCATION_SITE,
Uppercase I of If.

https://codereview.chromium.org/11817017/diff/39004/src/codegen.h#newcode101
src/codegen.h:101: static void
GenerateMapChangeElementsTransition(MacroAssembler* masm,
In function declarations, put the arguments right below each other:

static void GenerateMap...(MacroAssembler* masm,
                           AllocationSiteMode mode,
                           Label* allocation_site_info_found);

https://codereview.chromium.org/11817017/diff/39004/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/11817017/diff/39004/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5303: AllocationSiteMode allocation_site_mode =
instr->hydrogen()->
AllocationSiteMode allocation_site_mode =
    intr->hydrogen()->allocation_site_mode();

https://codereview.chromium.org/11817017/diff/39004/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11817017/diff/39004/src/objects.cc#newcode7527
src/objects.cc:7527: && (IsFastObjectElementsKind(to) ||
IsFastDoubleElementsKind(to))) {
&& on previous line. I'd also break the line between FLAG_track_allocation_sites
and IsFastSmiElementsKind.

[mvstanton, 2013-01-16 14:21:00]

thx for the careful look. Formatting updated (hopefully I'll remember this stuff
better too :)).

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

https://codereview.chromium.org/11817017/diff/39004/src/code-stubs.h#newcode454
src/code-stubs.h:454: class LengthBits: public BitField<int, 5, 5> {};
On 2013/01/16 13:35:55, Toon Verwaest wrote:
> You only need 4 bits here.

Done.

https://codereview.chromium.org/11817017/diff/39004/src/code-stubs.h#newcode457
src/code-stubs.h:457: STATIC_ASSERT(kMaximumClonedLength <= 8);
On 2013/01/16 13:35:55, Toon Verwaest wrote:
> With 4 bits, this field supports < 16.

Done.

https://codereview.chromium.org/11817017/diff/39004/src/codegen.h
File src/codegen.h (right):

https://codereview.chromium.org/11817017/diff/39004/src/codegen.h#newcode99
src/codegen.h:99: // if |mode| is set to DONT_TRACK_ALLOCATION_SITE,
On 2013/01/16 13:35:55, Toon Verwaest wrote:
> Uppercase I of If.

Done.

https://codereview.chromium.org/11817017/diff/39004/src/codegen.h#newcode101
src/codegen.h:101: static void
GenerateMapChangeElementsTransition(MacroAssembler* masm,
On 2013/01/16 13:35:55, Toon Verwaest wrote:
> In function declarations, put the arguments right below each other:
> 
> static void GenerateMap...(MacroAssembler* masm,
>                            AllocationSiteMode mode,
>                            Label* allocation_site_info_found);

Done, and updated the two functions below to have the same style. However, I
have to indent the lines of the GenerateMapChangeElementsTransition arguments
because they are too long for the 80 column character limit.

https://codereview.chromium.org/11817017/diff/39004/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/11817017/diff/39004/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5303: AllocationSiteMode allocation_site_mode =
instr->hydrogen()->
On 2013/01/16 13:35:55, Toon Verwaest wrote:
> AllocationSiteMode allocation_site_mode =
>     intr->hydrogen()->allocation_site_mode();

Done.

https://codereview.chromium.org/11817017/diff/39004/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11817017/diff/39004/src/objects.cc#newcode7527
src/objects.cc:7527: && (IsFastObjectElementsKind(to) ||
IsFastDoubleElementsKind(to))) {
On 2013/01/16 13:35:55, Toon Verwaest wrote:
> && on previous line. I'd also break the line between
FLAG_track_allocation_sites
> and IsFastSmiElementsKind.

Done.