Hi Toon, thanks for the review Friday. I've responded. We imagined some
functions that centralized policy around AllocationSiteInfo, and these are
the
GetMode(...) functions defined on that class, let me know your thoughts on
that.
Uh..I re-based and so now if you try to do a diff on just the later changes
you
get all this rebase crud...oops.
https://codereview.chromium.org/11817017/diff/18001/src/code-stubs.cc
File src/code-stubs.cc (right):
https://codereview.chromium.org/11817017/diff/18001/src/code-stubs.cc#newcode599
src/code-stubs.cc:599: // transition.
On 2013/01/11 16:56:06, mvstanton wrote:
Better, pass a parameter to the method indicating track/don'ttrack
(AllocationSiteInfoMode). And in as many places as possible, discover
this with
a helper method that hides:
1) the flag
2) to, from elements kind
3) other dynamic cases...e.tc.
I have added these methods, such that the on/off flag is controlled from
a central point, as well as the "business logic" of deciding whether a
particular (from,to) transition situation should be tracked or not.
https://codereview.chromium.org/11817017/diff/18001/src/code-stubs.h
File src/code-stubs.h (right):
https://codereview.chromium.org/11817017/diff/18001/src/code-stubs.h#newcode427
src/code-stubs.h:427: CLONE_ELEMENTS_WITH_ALLOCATION_SITE_INFO,
On 2013/01/11 16:56:06, mvstanton wrote:
Instead, use a bitfield that combines the AllocationSiteInfoMode with
the
FastCloneShallowArrayStub::Mode.
Done.
https://codereview.chromium.org/11817017/diff/18001/src/code-stubs.h#newcode448
src/code-stubs.h:448: static inline bool TrackAllocationSiteInfo(Mode
mode) {
On 2013/01/11 16:14:40, danno wrote:
nit: "Track" is a command, but you use it a query. How about
ShouldTrackAllocationSiteInfo?
Good point, thanks. I took Toon's advice and ended up dismantling this
section.
https://codereview.chromium.org/11817017/diff/18001/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):
https://codereview.chromium.org/11817017/diff/18001/src/ia32/ic-ia32.cc#newcode849
src/ia32/ic-ia32.cc:849:
ElementsTransitionGenerator::GenerateMapChangeElementsTransition(masm,
slow);
On 2013/01/11 16:56:06, mvstanton wrote:
Rename this parameter to handle_transition_in_runtime.
On reflection I think "slow" as in slow path and fast path is a pretty
established part of the vocabulary. Okay for me to let this one remain
as is? (just gimme a yes/no, no need for explainin').
https://codereview.chromium.org/11817017/diff/18001/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://codereview.chromium.org/11817017/diff/18001/src/ia32/lithium-codegen-ia32.cc#newcode5344
src/ia32/lithium-codegen-ia32.cc:5344: :
FastCloneShallowArrayStub::CLONE_ELEMENTS;
On 2013/01/11 16:56:06, mvstanton wrote:
Please put this in hydrogen-instructions just like what was done for
HFastLiteral (in fact move the key up to the base class
HMaterializedLiteral).
AND reverse this logic anyway.
By stripping the allocation_site_info flag from the
FastCloneShallowArrayStub::Mode enumeration, I was able to restore this
code to what it was before. allocation_site_info_mode is determined
upstream now in hydrogen.
https://codereview.chromium.org/11817017/diff/18001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/11817017/diff/18001/src/objects.cc#newcode10438
src/objects.cc:10438: ElementsKindToString(to_kind));
On 2013/01/11 16:56:06, mvstanton wrote:
spaces between 8*1024.
also AllocationSiteInfo->Print().
Got the spaces. On reflection, I'd rather not use
AllocationSiteInfo->Print() here. In the future checkins, Print() will
be developed, because AllocationSiteInfo will become more complex (right
now it's just a pointer to a boilerplate). Also, Print() wouldn't be
super helpful because I want to print out a change of state here...a
from->to transition. At the most, a Print() method on the
AllocationSiteInfo could inquire about the elements kind of the
boilerplate and print that...it wouldn't know about the transition.
But I will keep in mind the direction for the next checkins: try to
centralize printing stuff on the AllocationSiteInfo object. Will that be
okay?
https://codereview.chromium.org/11817017/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
