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#newcode500
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-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();
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#newcode1464
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#newcode5635
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#newcode486
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?
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.
