Thanks for the feedback Danno, happy you couldn't resist a look :). Let me
ask
Toon if he is free...
https://codereview.chromium.org/11817017/diff/6001/src/arm/codegen-arm.cc
File src/arm/codegen-arm.cc (right):
https://codereview.chromium.org/11817017/diff/6001/src/arm/codegen-arm.cc#newcode147
src/arm/codegen-arm.cc:147: MacroAssembler* masm, Label* fail) {
On 2013/01/10 22:58:59, danno wrote:
nit: here and elsewhere it might be clearer if you called the label
found_allocation_site_info rather than fail.
Done.
https://codereview.chromium.org/11817017/diff/6001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):
https://codereview.chromium.org/11817017/diff/6001/src/arm/lithium-codegen-arm.cc#newcode5476
src/arm/lithium-codegen-arm.cc:5476: // all the time?
On 2013/01/10 22:58:59, danno wrote:
Be careful (here and on other platforms). Originally when I wrote this
code, I
only had the CLONE_ANY_ELEMENTS version. When it landed in Chromium, I
tanked
performance in the page cycler performance bots and had to make sure
that the
CLONE_ELEMENTS and CLONE_DOUBLE_ELEMENTS were used whenever possible.
May it's
time to separate FastCloneShallowArrayStub::Mode and the flag to
create site
info always into to fields, and generate 6 versions of the copy stub,
CLONE_DOUBLE, CLONE_ELEMENTS, CLONE_ANY in two versions, one with
AllocationInfo, one without.
Done.
https://codereview.chromium.org/11817017/diff/6001/src/arm/lithium-codegen-arm.h
File src/arm/lithium-codegen-arm.h (right):
https://codereview.chromium.org/11817017/diff/6001/src/arm/lithium-codegen-arm.h#newcode369
src/arm/lithium-codegen-arm.h:369: bool create_allocation_site_info =
false);
On 2013/01/10 22:58:59, danno wrote:
Use the AllocationSiteInfoMode enum here instead of a boolean.
Done.
https://codereview.chromium.org/11817017/diff/6001/src/codegen.h
File src/codegen.h (right):
https://codereview.chromium.org/11817017/diff/6001/src/codegen.h#newcode101
src/codegen.h:101: Label* fail = NULL);
On 2013/01/10 22:58:59, danno wrote:
Maybe it makes sense to not provide a default value for fail (which
should be
called allocation_site_info_found), it forces callees to consciously
decide
(forgetting the parameter could cause subtle bugs).
Done.
https://codereview.chromium.org/11817017/diff/6001/src/heap.cc
File src/heap.cc (right):
https://codereview.chromium.org/11817017/diff/6001/src/heap.cc#newcode4398
src/heap.cc:4398: if (track_origin && InNewSpace(clone)) {
On 2013/01/10 22:58:59, danno wrote:
This is a bit dicey. Although I don't see a reason that the two
allocations (the
object above and the AllocationSiteInfo) won't be contiguous, it's a
bit
fragile. If track_origin is false, you should call AllocateRaw as
before. If
it's true, you should check object_size against:
Heap* heap = isolate()->heap();
const int kMinFreeNewSpaceAfterGC = heap->InitialSemiSpaceSize() *
3/4;
and if it's smaller, call AllocateInNewSpace for both objects
together, it
_must_ succeed in this case. In the case that object_size is greater
than
kMinFreeNewSpaceAfterGC, don't allocate the AllocationSiteInfo at all.
Also note that in this code you are allocation AllocationSiteInfos for
all
objects you allocate, not just JSArrays. Maybe you want add a
predicate
CanTrackAllocationSite to Map that returns "true" for JSArray maps,
and then
change the track_origin predicate above to contain "&&
source->map()->CanTrackAllocationSite()
Thanks, good advice. I actually removed the FLAG_track_allocation_sites,
as it seems to be everywhere. I figure that if someone has asked for the
feature to be on as a parameter to the function call, like here, then
any flag checking is upstream.
https://codereview.chromium.org/11817017/diff/6001/src/heap.cc#newcode4429
src/heap.cc:4429: // TODO(mvstanton): I don't understand this payload?
the original object?
On 2013/01/10 22:58:59, danno wrote:
Yes. And as discussed today, this is a bit wrong right now, it kind of
assumes
that source is a boilerplate array because this code path gated by
track_origin
is only used to copy boilerplate arrays (see my code above to limit
tracking to
just JSArrays).
When you copy deep boilerplates, you will have to pass in the
AllocationSite to
use rather than the array itself.
Right. For the moment, the boilerplate is exactly the right payload.
Later, when we move to AllocationMomentos, it will live in the
AllocationSite object instead. For this checkin I just remove the TODO.
https://codereview.chromium.org/11817017/diff/6001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
https://codereview.chromium.org/11817017/diff/6001/src/hydrogen-instructions.h#newcode4992
src/hydrogen-instructions.h:4992: bool create_allocation_site_info =
false)
On 2013/01/10 22:58:59, danno wrote:
Use AllocationSiteInfoMode.
Done.
https://codereview.chromium.org/11817017/diff/6001/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://codereview.chromium.org/11817017/diff/6001/src/ia32/lithium-codegen-ia32.cc#newcode5430
src/ia32/lithium-codegen-ia32.cc:5430: if (create_allocation_site_info)
{
On 2013/01/10 22:58:59, danno wrote:
Here and other platforms, only do this if and
"create_allocation_site_info &&
object->map()->CanTrackAllocationInfo()";
Done.
https://codereview.chromium.org/11817017/diff/6001/src/mips/stub-cache-mips.cc
File src/mips/stub-cache-mips.cc (right):
https://codereview.chromium.org/11817017/diff/6001/src/mips/stub-cache-mips.cc#newcode3410
src/mips/stub-cache-mips.cc:3410: a3, Operand(receiver_maps->at(i)));
On 2013/01/10 22:58:59, danno wrote:
Stray whitespace change?
Done.
https://codereview.chromium.org/11817017/diff/6001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/11817017/diff/6001/src/objects.cc#newcode10423
src/objects.cc:10423: if (info->payload()->IsJSArray()) {
On 2013/01/10 22:58:59, danno wrote:
You might not need this additional checks yet if you avoid creating
the
AllocationSiteInfos for non array types for now.
Ah, this if is a relic (or forebearer?) of the changes to follow, where
we cased on whether the payload was an array or a globalproperty object
to distinguish between the literal and constructor cases. I guess I can
just assert the condition here. Later, in fact it will change again to
be an AllocationSite object.
https://codereview.chromium.org/11817017/diff/6001/src/runtime.cc
File src/runtime.cc (right):
https://codereview.chromium.org/11817017/diff/6001/src/runtime.cc#newcode672
src/runtime.cc:672: TRACK_ALLOCATION_SITE_INFO
On 2013/01/10 22:58:59, danno wrote:
nit: ? goes on following line. i.e.
AllocationSiteInfoMode mode = FLAG_track_allocation_sites
? TRACK_ALLOCATION_SITE_INFO
: DONT_TRACK_ALLOCATION_SITE_INFO;
Done.
https://codereview.chromium.org/11817017/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
