Here's a first round. Sorry, couldn't resist... ;-)
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) { nit: here and elsewhere it might be clearer if you called the label found_allocation_site_info rather than fail. 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? 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. 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); Use the AllocationSiteInfoMode enum here instead of a boolean. 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); 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). 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)) { 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() 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? 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. 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) Use AllocationSiteInfoMode. 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) { Here and other platforms, only do this if and "create_allocation_site_info && object->map()->CanTrackAllocationInfo()"; 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))); Stray whitespace change? 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()) { You might not need this additional checks yet if you avoid creating the AllocationSiteInfos for non array types for now. 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 nit: ? goes on following line. i.e. AllocationSiteInfoMode mode = FLAG_track_allocation_sites ? TRACK_ALLOCATION_SITE_INFO : DONT_TRACK_ALLOCATION_SITE_INFO; https://codereview.chromium.org/11817017/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
