Hi Hannes, thx for the review, here is the update.
https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scopes.cc
File src/allocation-site-scopes.cc (right):
https://codereview.chromium.org/24250005/diff/27001/src/allocation-site-scopes.cc#newcode28
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-scopes.cc#newcode29
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-scopes.cc#newcode40
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-scopes.cc#newcode99
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-scopes.cc#newcode102
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-scopes.cc#newcode128
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-scopes.h#newcode33
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-scopes.cc
File src/allocation-site-scopes.cc (right):
https://codereview.chromium.org/24250005/diff/36001/src/allocation-site-scopes.cc#newcode29
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-scopes.cc#newcode40
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-scopes.cc#newcode62
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-scopes.cc#newcode99
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-scopes.cc#newcode102
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-scopes.cc#newcode105
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.
https://codereview.chromium.org/24250005/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
