AllocationSites for all literals

AllocationSites for all literals

BUG=
R=hpayer@chromium.org

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

[mvstanton, 2013-10-04 09:10:18]

Hi Hannes, here is the nested literal allocation site support. thx,
--Michael

[Hannes Payer (out of office), 2013-10-11 12:55:43]

First round of comments, already looking awesome.

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scope...
File src/allocation-site-scopes.cc (right):

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scope...
src/allocation-site-scopes.cc:28: #include "allocation-site-scopes.h"
newline

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scope...
src/allocation-site-scopes.cc:29: #define PTR(x) reinterpret_cast<void*>((x))
I am not a fan of macros, can we just use the cast below?

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scope...
src/allocation-site-scopes.cc:40: // top level
Make a proper comment out of this comment or remove it.

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scope...
src/allocation-site-scopes.cc:99: // Advance current if activated
finish with "."

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scope...
src/allocation-site-scopes.cc:102: // Something is wrong if we advance to the
end of the list here
finish with "."

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scope...
src/allocation-site-scopes.cc:128: // in the allocation site
finish with "."

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scopes.h
File src/allocation-site-scopes.h (right):

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scope...
src/allocation-site-scopes.h:33: #include "objects.h"
alphabetic order

https://codereview.chromium.org/24250005/diff/27001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/24250005/diff/27001/src/hydrogen.cc#newcode8562
src/hydrogen.cc:8562: // site_context->top()->GetPretenureMode().
Cool!

https://codereview.chromium.org/24250005/diff/27001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/24250005/diff/27001/src/objects.cc#newcode5634
src/objects.cc:5634: // 1) we have a JSArray, and
What about JSObject?

https://codereview.chromium.org/24250005/diff/27001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/24250005/diff/27001/src/objects.h#newcode2029
src/objects.h:2029: class AllocationSiteContext;
Move this one up to the other forward class declarations.

https://codereview.chromium.org/24250005/diff/27001/src/objects.h#newcode7910
src/objects.h:7910: return transition_info()->IsJSArray() ||
transition_info()->IsJSObject();
The method name and the description is outdated.

https://codereview.chromium.org/24250005/diff/27001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/24250005/diff/27001/src/runtime.cc#newcode516
src/runtime.cc:516: AllocationSiteUsageScope site_scope(&site_context, site,
boilerplate);
As discussed offline, right now we would emit mementos also in optimized code.
We should turn that off (in a different cl if it does not tank performance).

https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scope...
File src/allocation-site-scopes.cc (right):

https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scope...
src/allocation-site-scopes.cc:29: #define PTR(x) reinterpret_cast<void*>((x))
Again this macro, I would prefer using the cast. Or if you really like the
macro, add it to a global include file.

https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scope...
src/allocation-site-scopes.cc:40: // top level
Make a proper comment out of it.

https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scope...
src/allocation-site-scopes.cc:62: *context->current_.location() = *new_site;
Can we use a setter for that?

https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scope...
src/allocation-site-scopes.cc:99: // Advance current if activated
finish comment with "."

https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scope...
src/allocation-site-scopes.cc:102: // Something is wrong if we advance to the
end of the list here
finish comment with "."

https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scope...
src/allocation-site-scopes.cc:105: *context->current_.location() = casted_site;
Can we use a setter for that?

[mvstanton, 2013-10-11 13:41:47]

Hi Hannes, thx for the review, here is the update.

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scope...
File src/allocation-site-scopes.cc (right):

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scope...
src/allocation-site-scopes.cc:28: #include "allocation-site-scopes.h"
On 2013/10/11 12:55:44, Hannes Payer wrote:
> newline

Done.

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scope...
src/allocation-site-scopes.cc:29: #define PTR(x) reinterpret_cast<void*>((x))
On 2013/10/11 12:55:44, Hannes Payer wrote:
> I am not a fan of macros, can we just use the cast below?

Done.

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scope...
src/allocation-site-scopes.cc:40: // top level
On 2013/10/11 12:55:44, Hannes Payer wrote:
> Make a proper comment out of this comment or remove it.

Done.

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scope...
src/allocation-site-scopes.cc:99: // Advance current if activated
On 2013/10/11 12:55:44, Hannes Payer wrote:
> finish with "."

Done.

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scope...
src/allocation-site-scopes.cc:102: // Something is wrong if we advance to the
end of the list here
On 2013/10/11 12:55:44, Hannes Payer wrote:
> finish with "."

Done.

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scope...
src/allocation-site-scopes.cc:128: // in the allocation site
On 2013/10/11 12:55:44, Hannes Payer wrote:
> finish with "."

Done.

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scopes.h
File src/allocation-site-scopes.h (right):

https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scope...
src/allocation-site-scopes.h:33: #include "objects.h"
On 2013/10/11 12:55:44, Hannes Payer wrote:
> alphabetic order

Done.

https://codereview.chromium.org/24250005/diff/27001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/24250005/diff/27001/src/hydrogen.cc#newcode8562
src/hydrogen.cc:8562: // site_context->top()->GetPretenureMode().
On 2013/10/11 12:55:44, Hannes Payer wrote:
> Cool!

Yessir...velly nice.

https://codereview.chromium.org/24250005/diff/27001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/24250005/diff/27001/src/objects.cc#newcode5634
src/objects.cc:5634: // 1) we have a JSArray, and
On 2013/10/11 12:55:44, Hannes Payer wrote:
> What about JSObject?

At this moment in the code, we only emit mementos for Arrays with certain
characteristics. It's the next step to open the door to emit them for JSObjects
(this will need a change in AllocationSite::CanTrack() as we discussed:
currently that method only accepts arrays).

https://codereview.chromium.org/24250005/diff/27001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/24250005/diff/27001/src/objects.h#newcode2029
src/objects.h:2029: class AllocationSiteContext;
On 2013/10/11 12:55:44, Hannes Payer wrote:
> Move this one up to the other forward class declarations.

Done.

https://codereview.chromium.org/24250005/diff/27001/src/objects.h#newcode7910
src/objects.h:7910: return transition_info()->IsJSArray() ||
transition_info()->IsJSObject();
On 2013/10/11 12:55:44, Hannes Payer wrote:
> The method name and the description is outdated.

Updated comment and changed to SitePointsToLiteral() which is more logical.

https://codereview.chromium.org/24250005/diff/27001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/24250005/diff/27001/src/runtime.cc#newcode516
src/runtime.cc:516: AllocationSiteUsageScope site_scope(&site_context, site,
boilerplate);
On 2013/10/11 12:55:44, Hannes Payer wrote:
> As discussed offline, right now we would emit mementos also in optimized code.
> We should turn that off (in a different cl if it does not tank performance).

Good catch. I added this TODO in HOptimizedGraphBuilder::VisitObjectLiteral and
VisitArrayLiteral:

// TODO(mvstanton): Add a flag to turn off creation of any
// AllocationMementos for this call: we are in crankshaft and should have
// learned enough about transition behavior to stop emitting mementos.    

Performance-wise, I think we are okay at the moment because the mementos are
only turned on for:

1) Any nested or standalone Fast Smi Arrays, which also
2) don't meet the criteria of IsFastLiteral()

It should be a corner case. I see by instrumenting kraken that only a few tests
emit this code and only stanford-crypto-pbkdf2 makes any actual calls on it (6
times).

Still, the todos will be addressed to ensure a simpler system and to protect
performance in the pretenuring case, when all literals will get mementos from
this call in ordinary circumstances.

https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scope...
File src/allocation-site-scopes.cc (right):

https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scope...
src/allocation-site-scopes.cc:29: #define PTR(x) reinterpret_cast<void*>((x))
On 2013/10/11 12:55:44, Hannes Payer wrote:
> Again this macro, I would prefer using the cast. Or if you really like the
> macro, add it to a global include file.

Done.

https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scope...
src/allocation-site-scopes.cc:40: // top level
On 2013/10/11 12:55:44, Hannes Payer wrote:
> Make a proper comment out of it.

Done.

https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scope...
src/allocation-site-scopes.cc:62: *context->current_.location() = *new_site;
On 2013/10/11 12:55:44, Hannes Payer wrote:
> Can we use a setter for that?

Done.

https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scope...
src/allocation-site-scopes.cc:99: // Advance current if activated
On 2013/10/11 12:55:44, Hannes Payer wrote:
> finish comment with "."

Done.

https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scope...
src/allocation-site-scopes.cc:102: // Something is wrong if we advance to the
end of the list here
On 2013/10/11 12:55:44, Hannes Payer wrote:
> finish comment with "."

Done.

https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scope...
src/allocation-site-scopes.cc:105: *context->current_.location() = casted_site;
On 2013/10/11 12:55:44, Hannes Payer wrote:
> Can we use a setter for that?

Done.

[Hannes Payer (out of office), 2013-10-11 17:53:26]

https://codereview.chromium.org/24250005/diff/44001/src/allocation-site-scopes.h
File src/allocation-site-scopes.h (right):

https://codereview.chromium.org/24250005/diff/44001/src/allocation-site-scope...
src/allocation-site-scopes.h:40: class AllocationSiteContext {
Would it make sense to make AllocationSiteContext the base class of the Creation
and Usage Scope?

[mvstanton, 2013-10-14 14:31:42]

Thx for the comments, here is my update.

https://codereview.chromium.org/24250005/diff/44001/src/allocation-site-scopes.h
File src/allocation-site-scopes.h (right):

https://codereview.chromium.org/24250005/diff/44001/src/allocation-site-scope...
src/allocation-site-scopes.h:40: class AllocationSiteContext {
On 2013/10/11 17:53:27, Hannes Payer wrote:
> Would it make sense to make AllocationSiteContext the base class of the
Creation
> and Usage Scope?

Indeed, that works nicely.

[Hannes Payer (out of office), 2013-10-15 13:53:20]

Awesome, LGTM, just 2 nits

https://codereview.chromium.org/24250005/diff/54001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/24250005/diff/54001/src/objects.cc#newcode5641
src/objects.cc:5641: // of the object if desired.
take advantage of 80 chars

https://codereview.chromium.org/24250005/diff/54001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/24250005/diff/54001/src/objects.h#newcode875
src/objects.h:875: class AllocationSite;
alphabetic order

[mvstanton, 2013-10-15 14:53:14]

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