Supporting AllocationSiteInfo for Nested arrays

Supporting AllocationSiteInfo for Nested arrays


BUG=

[mvstanton, 2013-02-06 16:09:09]

Hi Danno, here is the nested array changeset, without platform ports. I'll have
those tomorrow.
Thanks for having a look!
--Michael

[danno, 2013-02-08 13:44:38]

Here some initial comments.

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

https://codereview.chromium.org/12114054/diff/8001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:413: // [sp]: flags (ignored)
I don't think you need to pass flags explicitly. Instead, parameterize the flags
into the MinorKey of the stub, keeping the same calling convention as before.
Create then two versions, one that passes ALLOCATE_SITE_INFO to the tail call
into the runtime, and one that doesn't. At the callee site, you know which
version you need, but you don't have to pay the extra expense of always
pushing/popping the ignored parameter in the fast case.

https://codereview.chromium.org/12114054/diff/8001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:487: __ bind(&slow_case);
In the parameterized stub, you'd have to push the flags here before calling
through.

https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.c...
src/arm/full-codegen-arm.cc:1594: (loop_depth() != 0 ||
!scope()->is_global_scope())) {
Add this as a predicate somewhere?

https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.c...
src/arm/full-codegen-arm.cc:1724: (loop_depth() != 0 ||
!scope()->is_global_scope())) {
Add this as a predicate somewhere?

https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.c...
src/arm/full-codegen-arm.cc:1746: length);
With the parameterized stub, don't push the flags here.

https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.c...
src/arm/full-codegen-arm.cc:1750: } else if (expr->depth() > 1) {
but push them here...

https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.c...
src/arm/full-codegen-arm.cc:1752: } else if (length >
FastCloneShallowArrayStub::kMaximumClonedLength) {
and here...

https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.c...
src/arm/full-codegen-arm.cc:1755:
ASSERT(IsFastSmiOrObjectElementsKind(constant_elements_kind) ||
But not here...

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

https://codereview.chromium.org/12114054/diff/8001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:5609: // memory, right?
No, I don't think so, since if the array already contained objects at this
level, it must have already been FAST_ELEMENTS of some form, which would prevent
it from transition to double. You can add an assert above somewhere that
ASSERT(original_object->HasFastObjectElements()) and remove this comment.

https://codereview.chromium.org/12114054/diff/8001/src/handles.h
File src/handles.h (right):

https://codereview.chromium.org/12114054/diff/8001/src/handles.h#newcode329
src/handles.h:329: 
unneeded whitespace change?

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

https://codereview.chromium.org/12114054/diff/8001/src/hydrogen-instructions....
src/hydrogen-instructions.h:5171: return original_boilerplate_; }
nit: } should be on next line

https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5290
src/hydrogen.cc:5290: bool allocation_sites_enabled) {
Maybe also pass this boolean flag around as a AllocationSiteMode, too?

https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5294
src/hydrogen.cc:5294: if (allocation_sites_enabled && boilerplate->IsJSArray())
{
boilerplate->ShouldTrackAllocationInfo instead?, removing much of the next few
lines.

https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5370
src/hydrogen.cc:5370: current_block()->LoopNestingDepth() == 0;
Maybe make this a predicate somewhere since you call it here and in full code
gen?

https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5492
src/hydrogen.cc:5492: current_block()->LoopNestingDepth() == 0;
predicate for this

https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5525
src/hydrogen.cc:5525: mode == TRACK_ALLOCATION_SITE)) {
Pass mode rather than a boolean

[danno, 2013-02-08 13:45:00]

Oh, BTW, all the ARM comments apply to the other platforms as well.

[mvstanton, 2013-02-11 11:11:24]

Thanks for the review Danno, here are my changes.

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

https://codereview.chromium.org/12114054/diff/8001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:413: // [sp]: flags (ignored)
On 2013/02/08 13:44:38, danno wrote:
> I don't think you need to pass flags explicitly. Instead, parameterize the
flags
> into the MinorKey of the stub, keeping the same calling convention as before.
> Create then two versions, one that passes ALLOCATE_SITE_INFO to the tail call
> into the runtime, and one that doesn't. At the callee site, you know which
> version you need, but you don't have to pay the extra expense of always
> pushing/popping the ignored parameter in the fast case. 

good idea, thanks.

https://codereview.chromium.org/12114054/diff/8001/src/arm/code-stubs-arm.cc#...
src/arm/code-stubs-arm.cc:487: __ bind(&slow_case);
On 2013/02/08 13:44:38, danno wrote:
> In the parameterized stub, you'd have to push the flags here before calling
> through.

Done.

https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.cc
File src/arm/full-codegen-arm.cc (right):

https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.c...
src/arm/full-codegen-arm.cc:1594: (loop_depth() != 0 ||
!scope()->is_global_scope())) {
On 2013/02/08 13:44:38, danno wrote:
> Add this as a predicate somewhere?

Done.

https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.c...
src/arm/full-codegen-arm.cc:1724: (loop_depth() != 0 ||
!scope()->is_global_scope())) {
On 2013/02/08 13:44:38, danno wrote:
> Add this as a predicate somewhere?

Done.

https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.c...
src/arm/full-codegen-arm.cc:1746: length);
On 2013/02/08 13:44:38, danno wrote:
> With the parameterized stub, don't push the flags here.

Done.

https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.c...
src/arm/full-codegen-arm.cc:1750: } else if (expr->depth() > 1) {
On 2013/02/08 13:44:38, danno wrote:
> but push them here...

Done.

https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.c...
src/arm/full-codegen-arm.cc:1752: } else if (length >
FastCloneShallowArrayStub::kMaximumClonedLength) {
On 2013/02/08 13:44:38, danno wrote:
> and here...

Done.

https://codereview.chromium.org/12114054/diff/8001/src/arm/full-codegen-arm.c...
src/arm/full-codegen-arm.cc:1755:
ASSERT(IsFastSmiOrObjectElementsKind(constant_elements_kind) ||
On 2013/02/08 13:44:38, danno wrote:
> But not here...

Done.

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

https://codereview.chromium.org/12114054/diff/8001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:5609: // memory, right?
On 2013/02/08 13:44:38, danno wrote:
> No, I don't think so, since if the array already contained objects at this
> level, it must have already been FAST_ELEMENTS of some form, which would
prevent
> it from transition to double. You can add an assert above somewhere that
> ASSERT(original_object->HasFastObjectElements()) and remove this comment.

Good point, thx.

https://codereview.chromium.org/12114054/diff/8001/src/handles.h
File src/handles.h (right):

https://codereview.chromium.org/12114054/diff/8001/src/handles.h#newcode329
src/handles.h:329: 
On 2013/02/08 13:44:38, danno wrote:
> unneeded whitespace change?

Done.

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

https://codereview.chromium.org/12114054/diff/8001/src/hydrogen-instructions....
src/hydrogen-instructions.h:5171: return original_boilerplate_; }
On 2013/02/08 13:44:38, danno wrote:
> nit: } should be on next line

Done.

https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5290
src/hydrogen.cc:5290: bool allocation_sites_enabled) {
On 2013/02/08 13:44:38, danno wrote:
> Maybe also pass this boolean flag around as a AllocationSiteMode, too?

Done. This area felt a little awkward because I had to distinguish between a
higher level decision to "do or not do" the tracking for the whole literal, and
then within the literal keep deciding on a member-by-member basis.

https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5294
src/hydrogen.cc:5294: if (allocation_sites_enabled && boilerplate->IsJSArray())
{
On 2013/02/08 13:44:38, danno wrote:
> boilerplate->ShouldTrackAllocationInfo instead?, removing much of the next few
> lines.

It's helpful in other places in the code too, further encapsulating the logic
about which kind of arrays should have the feature, thx!

https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5370
src/hydrogen.cc:5370: current_block()->LoopNestingDepth() == 0;
On 2013/02/08 13:44:38, danno wrote:
> Maybe make this a predicate somewhere since you call it here and in full code
> gen?

I made it a predicate on the FullCodeGenerator and the GraphBuilder, since the
information is structured a bit differently (the loop nesting depth).

https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5492
src/hydrogen.cc:5492: current_block()->LoopNestingDepth() == 0;
On 2013/02/08 13:44:38, danno wrote:
> predicate for this

Done.

https://codereview.chromium.org/12114054/diff/8001/src/hydrogen.cc#newcode5525
src/hydrogen.cc:5525: mode == TRACK_ALLOCATION_SITE)) {
On 2013/02/08 13:44:38, danno wrote:
> Pass mode rather than a boolean

Done.

[danno, 2013-02-11 15:05:21]

I like the direction this is going. Getting pretty close.

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

https://codereview.chromium.org/12114054/diff/14001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:500: // Add a flags argument, carefully preserving the
return address on the stack.
The comment is a bit misleading for ARM, since the return address is in "lr", so
there is no care needed :-)

https://codereview.chromium.org/12114054/diff/14001/src/arm/lithium-codegen-a...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/12114054/diff/14001/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:843: bool lazy_deopt_needed = info()->IsStub();
This change and the changes in the following lines are no longer needed if you
rebase, the bug is fixed in bleeding_edge.

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

https://codereview.chromium.org/12114054/diff/14001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:5730: if
(instr->hydrogen()->allocation_site_mode() == TRACK_ALLOCATION_SITE) {
If you check for FLAG_track_allocation_sites in the Hydrogen's
allocation_site_mode and returning DONT_TRACK_ALLOCATION_SITE, then you can
automatically not set kAllocationSiteInfoAllowed consistent with some of my
other comments.

https://codereview.chromium.org/12114054/diff/14001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/12114054/diff/14001/src/objects-inl.h#newcode...
src/objects-inl.h:1464: bool JSObject::ShouldTrackAllocationInfo() {
How abound adding:

if (!IsJSArray()) return true;

This centralizes the policy for tracking on all objects to this function,
hopefully obviating the need for the extra IsJSArray check at the caller sites
(please update those, too).

https://codereview.chromium.org/12114054/diff/14001/src/objects-inl.h#newcode...
src/objects-inl.h:5635: bool JSArray::ShouldTrackAllocationInfo() {
I'm a little confused how JSArray::ShouldTrackAllocationInfo differs from
JSObject::ShouldTrackAllocation info. If possible, I think JSObject's version
should "do it all", i.e. even casting to JSArray and check the GetMode result on
GetElementsKind(). The idea is that there is a single method on JSObject that
returns whether the SiteAllocationInfo should be allocated without doing any
additional checks on the object's instance type.

https://codereview.chromium.org/12114054/diff/14001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/12114054/diff/14001/src/objects.h#newcode8289
src/objects.h:8289: inline bool ShouldTrackAllocationInfo();
See previous comments, I think this method is superfluous.

https://codereview.chromium.org/12114054/diff/14001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/12114054/diff/14001/src/runtime.cc#newcode605
src/runtime.cc:605: (flags & ObjectLiteral::kAllocationSiteInfoAllowed) != 0;
The flag tells object literal instantiation to create allocation sites wherever
possible, so does it make sense to rename this flag to
"kCreateAllocationSiteInfos", since they are not allowed, they are desired, and
requested to be created if the boilerplate object supports them. If you agree,
rename the boolean variable appropriately.

https://codereview.chromium.org/12114054/diff/14001/src/runtime.cc#newcode606
src/runtime.cc:606: 
Also, perhaps you should fashion the calculation of this boolean value to
include "&& FLAG_track_allocation_sites" as you do later in this file for
consistency. Even better, to reduce the number of sites that need to check
"FLAG_track_allocation_sites", just make sure never to set the
"kAllocationSiteInfoAllowed" flag if FLAG_track_allocation_sites is false, then
you don't have to check the flag at the the sites in this file.

https://codereview.chromium.org/12114054/diff/14001/src/runtime.cc#newcode661
src/runtime.cc:661: mode);
nit: mode fits on previous line.

https://codereview.chromium.org/12114054/diff/14001/src/runtime.cc#newcode724
src/runtime.cc:724: boilerplate_object->ShouldTrackAllocationInfo()
nite: Just for clarity and easy, separate the boolean calculation into a
temporary variable then used in the ternary operator.

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

https://codereview.chromium.org/12114054/diff/14001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:486: int flags = allocation_site_mode_ ==
TRACK_ALLOCATION_SITE
Check FLAG_track_allocation_sites here, never setting kAllocationSiteInfoAllowed
(or the renamed version) if it's false.

[mvstanton, 2013-02-15 07:36:43]

Thanks Danno. Sorry I was late in addressing changes, somehow I missed your mail
of 3-4 days ago that you had a look at this!
--Michael

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

https://codereview.chromium.org/12114054/diff/14001/src/arm/code-stubs-arm.cc...
src/arm/code-stubs-arm.cc:500: // Add a flags argument, carefully preserving the
return address on the stack.
On 2013/02/11 15:05:21, danno wrote:
> The comment is a bit misleading for ARM, since the return address is in "lr",
so
> there is no care needed :-)

Done.

https://codereview.chromium.org/12114054/diff/14001/src/arm/lithium-codegen-a...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/12114054/diff/14001/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:843: bool lazy_deopt_needed = info()->IsStub();
On 2013/02/11 15:05:21, danno wrote:
> This change and the changes in the following lines are no longer needed if you
> rebase, the bug is fixed in bleeding_edge.

Done.

https://codereview.chromium.org/12114054/diff/14001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/12114054/diff/14001/src/objects-inl.h#newcode...
src/objects-inl.h:1464: bool JSObject::ShouldTrackAllocationInfo() {
On 2013/02/11 15:05:21, danno wrote:
> How abound adding:
> 
> if (!IsJSArray()) return true;
> 
> This centralizes the policy for tracking on all objects to this function,
> hopefully obviating the need for the extra IsJSArray check at the caller sites
> (please update those, too).

Done.

https://codereview.chromium.org/12114054/diff/14001/src/objects-inl.h#newcode...
src/objects-inl.h:5635: bool JSArray::ShouldTrackAllocationInfo() {
On 2013/02/11 15:05:21, danno wrote:
> I'm a little confused how JSArray::ShouldTrackAllocationInfo differs from
> JSObject::ShouldTrackAllocation info. If possible, I think JSObject's version
> should "do it all", i.e. even casting to JSArray and check the GetMode result
on
> GetElementsKind(). The idea is that there is a single method on JSObject that
> returns whether the SiteAllocationInfo should be allocated without doing any
> additional checks on the object's instance type.

I was thinking in terms of virtual overrides, where some object might bring
other factors into the decision. In the case of JSArray, it's a check on
ElementsKind...for other objects maybe it would be something different. I'll
consolidate this under JSObject, thx.

https://codereview.chromium.org/12114054/diff/14001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/12114054/diff/14001/src/runtime.cc#newcode605
src/runtime.cc:605: (flags & ObjectLiteral::kAllocationSiteInfoAllowed) != 0;
On 2013/02/11 15:05:21, danno wrote:
> The flag tells object literal instantiation to create allocation sites
wherever
> possible, so does it make sense to rename this flag to
> "kCreateAllocationSiteInfos", since they are not allowed, they are desired,
and
> requested to be created if the boilerplate object supports them. If you agree,
> rename the boolean variable appropriately.

"they are not allowed, they are desired." Beautiful :).

https://codereview.chromium.org/12114054/diff/14001/src/runtime.cc#newcode606
src/runtime.cc:606: 
On 2013/02/11 15:05:21, danno wrote:
> Also, perhaps you should fashion the calculation of this boolean value to
> include "&& FLAG_track_allocation_sites" as you do later in this file for
> consistency. Even better, to reduce the number of sites that need to check
> "FLAG_track_allocation_sites", just make sure never to set the
> "kAllocationSiteInfoAllowed" flag if FLAG_track_allocation_sites is false,
then
> you don't have to check the flag at the the sites in this file.

right on, done.

https://codereview.chromium.org/12114054/diff/14001/src/runtime.cc#newcode661
src/runtime.cc:661: mode);
On 2013/02/11 15:05:21, danno wrote:
> nit: mode fits on previous line.

Changes based on other recommendations affected this line differently.

https://codereview.chromium.org/12114054/diff/14001/src/runtime.cc#newcode724
src/runtime.cc:724: boilerplate_object->ShouldTrackAllocationInfo()
On 2013/02/11 15:05:21, danno wrote:
> nite: Just for clarity and easy, separate the boolean calculation into a
> temporary variable then used in the ternary operator.

Done.

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

https://codereview.chromium.org/12114054/diff/14001/src/x64/code-stubs-x64.cc...
src/x64/code-stubs-x64.cc:486: int flags = allocation_site_mode_ ==
TRACK_ALLOCATION_SITE
On 2013/02/11 15:05:21, danno wrote:
> Check FLAG_track_allocation_sites here, never setting
kAllocationSiteInfoAllowed
> (or the renamed version) if it's false.

With some other changes already present, I think I won't have to do that: the
allocation_site_mode_ variable won't be set to TRACK_ALLOCATION_SITE if the flag
is false. The flag guards all places where explicit direction to do something
allocation-site-wise is recorded in data structures, making it redundant to
check the flag again given an explicit directive. Does that sound like a good
approach?

[mvstanton, 2013-02-17 16:05:56]

Hi Danno, I moved the AllocationSiteInfo::GetMode() helpers to be inlined
functions. I noticed that this made a pretty big difference when I called it
from JSObject::ShouldTrackAllocationInfo() and they weren't inlined.

[mvstanton, 2013-02-21 11:01:09]

Hi Danno,
1) this patch includes the GC_count heuristic
2) I found a place in runtime.cc when we stored elements in literals that kind
of overlapped/duplicated the allocation site info policy. Removed that code.
3) I marked my helper functions in objects.h inline because it's appropriate.