Track elements_kind transitions in KeyedStoreICs.

Track elements_kind transitions in KeyedStoreICs.

Committed: http://code.google.com/p/v8/source/detail?r=9577

[Jakob Kummerow, 2011-10-06 08:05:38]

Please take a first look, especially at the ia32 specific bits. If those look
good, I'll port them to the other architectures.

[danno, 2011-10-07 09:16:38]

more to come....

http://codereview.chromium.org/8166017/diff/3001/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/8166017/diff/3001/src/heap.h#newcode204
src/heap.h:204: "KeyedStoreElmMonoTransSmiObject")                              
    \
Use the full symbol name

http://codereview.chromium.org/8166017/diff/3001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8166017/diff/3001/src/ic.cc#newcode1600
src/ic.cc:1600: if (ic_state == UNINITIALIZED || ic_state == PREMONOMORPHIC ||
How about just create a variable "use_monomorphic_stub" that gets set if
ic_state == UNINITIALIZED || ic_state == PREMONOMORPHIC is true, or in a
separate if for IsMonomorphicButMissingTransition, so that you don't have to
re-test ic_state == MONOMORPHIC && stub_kind > STORE_NO_TRANSITION

http://codereview.chromium.org/8166017/diff/3001/src/ic.cc#newcode1603
src/ic.cc:1603: target()->ContainsMap(receiver->map()))) {
Can you put these into a predicate or something that makes it clear what the
test is actually trying to accomplish? Like MonomorphicButMissingTransition.
Whatever.

http://codereview.chromium.org/8166017/diff/3001/src/ic.cc#newcode1604
src/ic.cc:1604: Map* existing_additional_map = NULL;
smi_only_or_double_map? Or at least comment the name.

http://codereview.chromium.org/8166017/diff/3001/src/ic.cc#newcode1759
src/ic.cc:1759: bool force_generic) {
stub_kind?

http://codereview.chromium.org/8166017/diff/3001/src/ic.h
File src/ic.h (right):

http://codereview.chromium.org/8166017/diff/3001/src/ic.h#newcode345
src/ic.h:345: enum StubKind { LOAD, STORE_NO_TRANSITION,
STORE_TRANSITION_SMI_TO_OBJECT,
nit: list these vertically, they are much easier to read.

http://codereview.chromium.org/8166017/diff/3001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/8166017/diff/3001/src/objects.cc#newcode2223
src/objects.cc:2223: ASSERT(safe_to_add_transition);
comment about this assert?

http://codereview.chromium.org/8166017/diff/3001/src/objects.cc#newcode2280
src/objects.cc:2280: if (current_map->elements_kind() == FAST_SMI_ONLY_ELEMENTS
&&
can you put this in "from_kind" and rename the other one to "to_kind"

http://codereview.chromium.org/8166017/diff/3001/src/objects.cc#newcode2346
src/objects.cc:2346: }
I think you need a test that covers these code paths.

http://codereview.chromium.org/8166017/diff/3001/src/objects.cc#newcode7536
src/objects.cc:7536: // TODO(jkummerow): Consider refactoring this to share some
code.
Do you still need this?

http://codereview.chromium.org/8166017/diff/3001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/8166017/diff/3001/src/objects.h#newcode3748
src/objects.h:3748: bool ContainsMap(Map* map);
Do you still need this?

[Jakob Kummerow, 2011-10-07 13:17:54]

http://codereview.chromium.org/8166017/diff/3001/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/8166017/diff/3001/src/heap.h#newcode204
src/heap.h:204: "KeyedStoreElmMonoTransSmiObject")                              
    \
On 2011/10/07 09:16:38, danno wrote:
> Use the full symbol name

Done.

http://codereview.chromium.org/8166017/diff/3001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8166017/diff/3001/src/ic.cc#newcode1600
src/ic.cc:1600: if (ic_state == UNINITIALIZED || ic_state == PREMONOMORPHIC ||
On 2011/10/07 09:16:38, danno wrote:
> How about just create a variable "use_monomorphic_stub" that gets set if
> ic_state == UNINITIALIZED || ic_state == PREMONOMORPHIC is true, or in a
> separate if for IsMonomorphicButMissingTransition, so that you don't have to
> re-test ic_state == MONOMORPHIC && stub_kind > STORE_NO_TRANSITION

Done.

http://codereview.chromium.org/8166017/diff/3001/src/ic.cc#newcode1603
src/ic.cc:1603: target()->ContainsMap(receiver->map()))) {
On 2011/10/07 09:16:38, danno wrote:
> Can you put these into a predicate or something that makes it clear what the
> test is actually trying to accomplish? Like MonomorphicButMissingTransition.
> Whatever.

Refactored this section.

http://codereview.chromium.org/8166017/diff/3001/src/ic.cc#newcode1604
src/ic.cc:1604: Map* existing_additional_map = NULL;
On 2011/10/07 09:16:38, danno wrote:
> smi_only_or_double_map? Or at least comment the name.

Done.

http://codereview.chromium.org/8166017/diff/3001/src/ic.cc#newcode1759
src/ic.cc:1759: bool force_generic) {
On 2011/10/07 09:16:38, danno wrote:
> stub_kind?

No, as discussed. Two reasons:
(1) The logic wouldn't become any easier, it would only be moved.
(2) Having the logic here means we don't have to go through a miss first, but
can generate a transition IC right away if we have to.

http://codereview.chromium.org/8166017/diff/3001/src/ic.h
File src/ic.h (right):

http://codereview.chromium.org/8166017/diff/3001/src/ic.h#newcode345
src/ic.h:345: enum StubKind { LOAD, STORE_NO_TRANSITION,
STORE_TRANSITION_SMI_TO_OBJECT,
On 2011/10/07 09:16:38, danno wrote:
> nit: list these vertically, they are much easier to read.

Done.

http://codereview.chromium.org/8166017/diff/3001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/8166017/diff/3001/src/objects.cc#newcode2223
src/objects.cc:2223: ASSERT(safe_to_add_transition);
On 2011/10/07 09:16:38, danno wrote:
> comment about this assert?

Done.

http://codereview.chromium.org/8166017/diff/3001/src/objects.cc#newcode2280
src/objects.cc:2280: if (current_map->elements_kind() == FAST_SMI_ONLY_ELEMENTS
&&
On 2011/10/07 09:16:38, danno wrote:
> can you put this in "from_kind" and rename the other one to "to_kind"

Done.

http://codereview.chromium.org/8166017/diff/3001/src/objects.cc#newcode2346
src/objects.cc:2346: }
On 2011/10/07 09:16:38, danno wrote:
> I think you need a test that covers these code paths.

Done.

http://codereview.chromium.org/8166017/diff/3001/src/objects.cc#newcode7536
src/objects.cc:7536: // TODO(jkummerow): Consider refactoring this to share some
code.
On 2011/10/07 09:16:38, danno wrote:
> Do you still need this?

No.

http://codereview.chromium.org/8166017/diff/3001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/8166017/diff/3001/src/objects.h#newcode3748
src/objects.h:3748: bool ContainsMap(Map* map);
On 2011/10/07 09:16:38, danno wrote:
> Do you still need this?

No.

[Jakob Kummerow, 2011-10-10 11:19:19]

In patch set 4, transition-capable ICs are now called MEGAMORPHIC. Do you like
this better?

[Jakob Kummerow, 2011-10-10 13:14:53]

Slava, can you please take a look at the GC related changes? (objects.h,
objects.cc below line 6000)

[danno, 2011-10-10 13:15:12]

LGTM with nits if you get Slava's OK on the GC code.

http://codereview.chromium.org/8166017/diff/15001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8166017/diff/15001/src/ic.cc#newcode1656
src/ic.cc:1656: if (stub_kind > STORE_NO_TRANSITION) {
For clarity, maybe factor out the following code into ConstructPolymorphicStub
and ConstructPolymorphicStubWithTransition?

http://codereview.chromium.org/8166017/diff/15001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/8166017/diff/15001/src/objects.cc#newcode6724
src/objects.cc:6724: bool allow_overwriting_maps) {
I'm not sure I like passing this bool for just debugging purposes.

http://codereview.chromium.org/8166017/diff/15001/src/x64/stub-cache-x64.cc
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/8166017/diff/15001/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:2656: return GetCode(NORMAL, NULL);
MEGAMORPHIC?

[Vyacheslav Egorov (Chromium), 2011-10-10 14:02:59]

http://codereview.chromium.org/8166017/diff/15001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/8166017/diff/15001/src/objects.cc#newcode6758
src/objects.cc:6758: Map* target = reinterpret_cast<Map*>(object);
My suggestion: instead of glue-ing together two transitions SMI -> DOUBLE ->
FAST, SMI -> FAST and making transitions graph into a dag (instead of tree) lets
consider always creating SMI -> DOUBLE -> FAST chain when transitioning from SMI
to FAST. 

That would waste some space (minuscule amount) but will simplify the system and
keep transition graph a tree (which means this code will not be needed and
reasoning about tree is always simpler).

[Jakob Kummerow, 2011-10-10 16:21:42]

Danno: please take another look.

Slava: You're off the hook, as I've removed the now-obsolete GC-related changes
again. Of course, feel free to take a look nevertheless if you wish :-)

http://codereview.chromium.org/8166017/diff/15001/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8166017/diff/15001/src/ic.cc#newcode1656
src/ic.cc:1656: if (stub_kind > STORE_NO_TRANSITION) {
On 2011/10/10 13:15:12, danno wrote:
> For clarity, maybe factor out the following code into ConstructPolymorphicStub
> and ConstructPolymorphicStubWithTransition?

Done. Called them "Compute..." to match the monomorphic methods.

http://codereview.chromium.org/8166017/diff/15001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/8166017/diff/15001/src/objects.cc#newcode6724
src/objects.cc:6724: bool allow_overwriting_maps) {
On 2011/10/10 13:15:12, danno wrote:
> I'm not sure I like passing this bool for just debugging purposes.

Done.

http://codereview.chromium.org/8166017/diff/15001/src/objects.cc#newcode6758
src/objects.cc:6758: Map* target = reinterpret_cast<Map*>(object);
On 2011/10/10 14:02:59, Vyacheslav Egorov wrote:
> My suggestion: instead of glue-ing together two transitions SMI -> DOUBLE ->
> FAST, SMI -> FAST and making transitions graph into a dag (instead of tree)
lets
> consider always creating SMI -> DOUBLE -> FAST chain when transitioning from
SMI
> to FAST. 
> 
> That would waste some space (minuscule amount) but will simplify the system
and
> keep transition graph a tree (which means this code will not be needed and
> reasoning about tree is always simpler).

Done.

http://codereview.chromium.org/8166017/diff/15001/src/x64/stub-cache-x64.cc
File src/x64/stub-cache-x64.cc (right):

http://codereview.chromium.org/8166017/diff/15001/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:2656: return GetCode(NORMAL, NULL);
On 2011/10/10 13:15:12, danno wrote:
> MEGAMORPHIC?

Done, also for ARM.

[danno, 2011-10-11 09:08:31]

lgtm, with nits.

http://codereview.chromium.org/8166017/diff/18002/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8166017/diff/18002/src/ic.cc#newcode1601
src/ic.cc:1601: stub_kind <= STORE_NO_TRANSITION) {
nit: Predacate? !IsTransitionStubKind(stub_kind)

http://codereview.chromium.org/8166017/diff/18002/src/ic.cc#newcode1629
src/ic.cc:1629: if (stub_kind > STORE_NO_TRANSITION) {
IsTransitionStubKind

http://codereview.chromium.org/8166017/diff/18002/src/ic.cc#newcode1656
src/ic.cc:1656: if (stub_kind > STORE_NO_TRANSITION) {
IsTransitionStubKind

[Jakob Kummerow, 2011-10-11 09:14:25]

Nits fixed, landing.

http://codereview.chromium.org/8166017/diff/18002/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8166017/diff/18002/src/ic.cc#newcode1601
src/ic.cc:1601: stub_kind <= STORE_NO_TRANSITION) {
On 2011/10/11 09:08:31, danno wrote:
> nit: Predacate? !IsTransitionStubKind(stub_kind)

Done.

http://codereview.chromium.org/8166017/diff/18002/src/ic.cc#newcode1629
src/ic.cc:1629: if (stub_kind > STORE_NO_TRANSITION) {
On 2011/10/11 09:08:31, danno wrote:
> IsTransitionStubKind

Done.

http://codereview.chromium.org/8166017/diff/18002/src/ic.cc#newcode1656
src/ic.cc:1656: if (stub_kind > STORE_NO_TRANSITION) {
On 2011/10/11 09:08:31, danno wrote:
> IsTransitionStubKind

Done.