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#newcode500
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-arm.cc
File src/arm/lithium-codegen-arm.cc (right):
https://codereview.chromium.org/12114054/diff/14001/src/arm/lithium-codegen-arm.cc#newcode843
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-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://codereview.chromium.org/12114054/diff/14001/src/ia32/lithium-codegen-ia32.cc#newcode5730
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#newcode1464
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#newcode5635
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#newcode486
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.
https://codereview.chromium.org/12114054/
--
--
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.
