Better support for 'polymorphic' JS and external arrays

Better support for 'polymorphic' JS and external arrays

Allow  keyed store/load stubs to switch between external array and fast JS arrays without forcing a state transition to the generic stub.

There CL consists of two pieces of functionality. First, code stubs for fast element arrays don't immediately transition to the MEGAMORPHIC state when there's a map mismatch. Second, two ICs are cached per map for fast elements, the MONOMORPHIC version, and a new MEGAMORPHIC version that handles two or more different maps. Currently, the only array types supported by the MEGAMORPHIC stub are fast elements for objects and JSArrays.

BUG=
TEST=

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

[danno, 2011-04-27 14:56:28]

Please review.

[Mads Ager (chromium), 2011-04-27 15:57:21]

Very high-level comments for now.

I don't like the idea of creating new full stubs that handle all of the cases. I
would like to reuse monomorphic stubs instead. I see two ways to do that:

1. What we do for named access. Have a cache of monomorphic stubs that you probe
in generated code in the megamorphic case. That cache can be mined for type
feedback in crankshaft. The downside is that you do not have the name here to
filter the stubs that you collect as type feedback, so it might be hard to limit
the type feedback to something valid. If we can overcome that problem I think I
would prefer this solution.

2. Generate new polymorphic IC stubs, but ones that are simple and just does a
type switch with tail calls to monomorphic stubs. Alternatively, they could do
tail calls to a number of fixed stubs that can load from one type of element
given that the map has already been verified. We would then need a number of
those for the diferent element types.

[danno, 2011-04-28 12:01:54]

WIP, so don't worry about nits for now. Please take a look specifically at ic.cc
(for polymorphic keyed ic logic) and stub-cache-ia32.cc (for the polymorphic
stub generation).

[Mads Ager (chromium), 2011-04-28 13:02:01]

Only looked at load for now. This seems like the right approach.

We should get rid of the handlification introduced here. Mixing handles with
code that does not use handles is very dangerous and has led to a lot of trouble
before so we should avoid it.

As a future work, I think we could cut down on the space overhead even more here
if we know the type of elements for the maps (which I know we don't right now).
For a given element type, the only difference between the monomorphic stubs is
the initial map check. That check is now repeated in the megamorphic and the
monomorphic stub. We could generate a fixed number of stubs that load from each
of the different types of backing stores without checking the map. The
megamorphic ICs could then call these after their map checks instead of
generating new monomorphic stubs. This is only a suggestion for potential future
work. What you are doing here seems like the right thing for now.

http://codereview.chromium.org/6894003/diff/8040/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/6894003/diff/8040/src/ia32/stub-cache-ia32.cc#...
src/ia32/stub-cache-ia32.cc:763: Builtins::kKeyedLoadIC_MissForceGeneric);
Four space indent?

http://codereview.chromium.org/6894003/diff/8040/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6894003/diff/8040/src/ic.cc#newcode1081
src/ic.cc:1081: ZoneMapList* receiver_maps,
Do you have a zone at this point? Maybe just use normal C++ allocated lists for
these?

http://codereview.chromium.org/6894003/diff/8040/src/ic.cc#newcode1254
src/ic.cc:1254: HandleScope handle_scope(isolate());
This is a nasty mix of handlified and non-handlified code. I don't understand
why this is needed. We really should just set the stub and then return the
result of a runtime lookup.

http://codereview.chromium.org/6894003/diff/8040/src/ic.cc#newcode1583
src/ic.cc:1583: ZoneMapList* result = new
ZoneMapList(KeyedIC::kMaxKeyedPolymorphism);
This looks wrong. You have a local |result| here that you modify and then you
return the empty one in the outer scope?

http://codereview.chromium.org/6894003/diff/8040/src/ic.cc#newcode1611
src/ic.cc:1611: HandleScope handle_scope;
This looks really dangerous. Is this handle scope only there because you are
doing lists of handles to maps? In that case, please use lists of raw maps.
Allocations are not safe here so I would prefer not to use handlified code. That
way it is completely clear that you have to check for failures and deal with it
appropriately and not call handlified methods that will garbage collect.

http://codereview.chromium.org/6894003/diff/8040/src/ic.cc#newcode1633
src/ic.cc:1633: ZoneScope zone_scope(DELETE_ON_EXIT);
The lists here do not seem big enough to require zone allocation. I would just
use C++ allocation for them (see list.h).

http://codereview.chromium.org/6894003/diff/8040/src/ic.cc#newcode1634
src/ic.cc:1634: HandleScope handle_scope;
Let's not add HandleScopes here. That makes it very tempting to call handlified
methods which can garbage collect which is unsafe at this point. We need to
stick to raw pointers here to force ourselves to deal with allocation failures
directly.

http://codereview.chromium.org/6894003/diff/8040/src/ic.cc#newcode1677
src/ic.cc:1677: // Lookup all of the receiver maps in the cache, they should all
already
This introduces a dependency that I don't think we have had before. Namely that
map caches cannot be cleared unless you also clear IC stubs in all code. This is
true by default. We have flags that could break this. We should probably remove
those flags. (Something like cleanup-caches-in-maps-at-gc and cleanup-ics-at-gc.
Those can no longer be set independently. Clearing map caches at GC will have to
always imply clearing ICs at GC.)

[Jakob Kummerow, 2011-05-09 14:58:40]

A few drive-by comments... I haven't looked at everything in detail yet, but
these caught my eye:

http://codereview.chromium.org/6894003/diff/16001/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/6894003/diff/16001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:2697: __ cmp(FieldOperand(edx,
HeapObject::kMapOffset), map);
s/edx/eax/?

also, why not use DispatchMap(eax, Handle<Map>(receiver_maps->at(current),
Handle<Code>(handler_ics->at(current), true)?

http://codereview.chromium.org/6894003/diff/16001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3151: __ JumpIfSmi(eax, &miss);
s/eax/edx/? (see original code: "test(edx, ...)")

http://codereview.chromium.org/6894003/diff/16001/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3157: __ cmp(FieldOperand(edx,
HeapObject::kMapOffset), map);
Again: why not use DispatchMap(...)?

[danno%chromium.org, 2011-05-10 07:24:06]

Ready to land, please review.

[Mads Ager (chromium), 2011-05-10 13:38:06]

Urgh, this change is getting massive. With the size of this I'm afraid I have
missed something. Let's see if we can chop changes into smaller parts going
forward. :-)

I have some comments. Definitely looks like this is getting there.

http://codereview.chromium.org/6894003/diff/35002/src/arm/ic-arm.cc
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/6894003/diff/35002/src/arm/ic-arm.cc#newcode1136
src/arm/ic-arm.cc:1136: : ExternalReference(IC_Utility(kKeyedStoreIC_Miss),
masm->isolate());
Really funky indentation. ;-)

http://codereview.chromium.org/6894003/diff/35002/src/arm/ic-arm.cc#newcode1153
src/arm/ic-arm.cc:1153: ExternalReference(IC_Utility(kKeyedStoreIC_Slow),
masm->isolate());
What is the idea with the slow case here? Goes directly to the runtime system
without being treated as a miss so that we do not perform an IC state
transition? So this is for when we bail out of a store ic because something that
has got nothing to do with the type of the object failed?

http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:3093: KeyedLoadIC keyed_load_ic(isolate());
Isn't this called from a KeyedLoadIC instance? Can't we pass in the other stub
to the generation instead? I find it a little strange to create a KeyedLoadIC
instance here.

http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:3095:
keyed_load_ic.ComputeMonomorphicBuiltinFastElementStub(
Are any of these stubs actually builtin? This is the dynamically generated stub
that does not perform map checks, right? Maybe find a name that does not contain
'Builtin'?

http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:3103: false);
Creating an enum for this would make this call site a bit easier to read.
CHECK_SMI/DONT_CHECK_SMI?

http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:3190: KeyedStoreIC keyed_store_ic(isolate());
I would prefer passing in the actual stub here and dealing with allocation
failures at the higher level at the caller. Creating KeyedStoreICs outside of
ic.cc seems a bit fishy to me.

Same comment for the other occurrences. :-)

http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:3785: __ ldr(r3, FieldMemOperand(receiver,
JSObject::kElementsOffset));
Have you always checked that the receiver is not a smi when you get here?

http://codereview.chromium.org/6894003/diff/35002/src/code-stubs.h
File src/code-stubs.h (right):

http://codereview.chromium.org/6894003/diff/35002/src/code-stubs.h#newcode993
src/code-stubs.h:993: class KeyedStoreStub : public CodeStub {
Why not just have this one and have a minor key for Object and one for Array?
They share code generation.

http://codereview.chromium.org/6894003/diff/35002/src/code-stubs.h#newcode1037
src/code-stubs.h:1037: Major MajorKey() {
Just don't implement Major and Minor key?

http://codereview.chromium.org/6894003/diff/35002/src/code-stubs.h#newcode1069
src/code-stubs.h:1069: #define DECLARE_EXTERNAL_ARRAY_STUB(op, type)            
              \
Do you really need all of these different types here. They share code
generation, right? So, why not just have ExternalArrayLoadStub with a minor key
that is the type (byte, unsigned, ...)?

http://codereview.chromium.org/6894003/diff/35002/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/6894003/diff/35002/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:718: Builtins::kKeyedLoadIC_MissForceGeneric);
four-space indent?

http://codereview.chromium.org/6894003/diff/35002/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3641: NearLabel done;
Accidental edit? There are more than this one I think.

http://codereview.chromium.org/6894003/diff/35002/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6894003/diff/35002/src/ic.cc#newcode1070
src/ic.cc:1070: if (is_js_array) {
If you simplify the stub hierarchy this can be just:

KeyedLoadStub stub(is_js_array);
return stub.TryGetCode();

http://codereview.chromium.org/6894003/diff/35002/src/ic.cc#newcode1082
src/ic.cc:1082: switch (array_type) {
ExternalArrayLoadStub stub(array_type);
return stub.TryGetCode();

?

http://codereview.chromium.org/6894003/diff/35002/src/ic.cc#newcode1656
src/ic.cc:1656: // the MONOMORPHIC stubs to jump into for all of the receiver
types that it
Is this comment correct any more? You might need the monomorphic stub for the
typed arrays to get the external array type, but these are not the stubs that
you use to compose the actual megamorphic stubs, right?

http://codereview.chromium.org/6894003/diff/35002/src/ic.cc#newcode1692
src/ic.cc:1692: Object* maybe_cached_stub =
receiver->map()->FindInCodeCache(megamorphic_name,
I'm not sure I understand why you cache this stub on the current receiver's map.
It seems to me that this can create a rather strange mix of types that are
unrelated to this load/store site? I'm concerned that the type feedback
generated here will be too polymorphic.

It seems to me that if we want to cache these stubs we should have some sort of
global stub cache instead of caching on one of the maps supported by the stub.
It would be nice to be able to share these by having a mapping from a list of
maps to a stub handling them.

http://codereview.chromium.org/6894003/diff/35002/src/ic.cc#newcode1811
src/ic.cc:1811: MaybeObject* maybe_update = receiver->UpdateMapCodeCache(name,
result);
It seems to me that something like this should be on the stub cache. In
particular, we need to make sure that the profiler knows about all generated
code. Not clear to me if it does? It doesn't follow the normal pattern at least,
but maybe it can't?

http://codereview.chromium.org/6894003/diff/35002/src/ic.h
File src/ic.h (right):

http://codereview.chromium.org/6894003/diff/35002/src/ic.h#newcode337
src/ic.h:337: virtual MaybeObject* ComputeMonomorphicBuiltinFastElementStub(
Remove the Builtin from the name since it is not a builtin but a normal stub?

[danno, 2011-05-11 14:20:19]

Addressed all your feedback and filed bug 1384 for moving the megamorphic cache
off the receiver map. Please take another look.

http://codereview.chromium.org/6894003/diff/35002/src/arm/ic-arm.cc
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/6894003/diff/35002/src/arm/ic-arm.cc#newcode1136
src/arm/ic-arm.cc:1136: : ExternalReference(IC_Utility(kKeyedStoreIC_Miss),
masm->isolate());
On 2011/05/10 13:38:06, Mads Ager wrote:
> Really funky indentation. ;-)

Done.

http://codereview.chromium.org/6894003/diff/35002/src/arm/ic-arm.cc#newcode1153
src/arm/ic-arm.cc:1153: ExternalReference(IC_Utility(kKeyedStoreIC_Slow),
masm->isolate());
Exactly. This happens in  the external arrays stubs in edge-case conversions or
some of the allocation failure paths. Since the type hasn't changed, it's
premature to change the IC to the generic case, since the generic stub will not
be any faster better. I've added appropriate comments.
  
On 2011/05/10 13:38:06, Mads Ager wrote:
> What is the idea with the slow case here? Goes directly to the runtime system
> without being treated as a miss so that we do not perform an IC state
> transition? So this is for when we bail out of a store ic because something
that
> has got nothing to do with the type of the object failed?

http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:3093: KeyedLoadIC keyed_load_ic(isolate());
On 2011/05/10 13:38:06, Mads Ager wrote:
> Isn't this called from a KeyedLoadIC instance? Can't we pass in the other stub
> to the generation instead? I find it a little strange to create a KeyedLoadIC
> instance here.

Done.

http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:3095:
keyed_load_ic.ComputeMonomorphicBuiltinFastElementStub(
On 2011/05/10 13:38:06, Mads Ager wrote:
> Are any of these stubs actually builtin? This is the dynamically generated
stub
> that does not perform map checks, right? Maybe find a name that does not
contain
> 'Builtin'?

Done.

http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:3103: false);
Really good idea. Mind if I do that in another CL? I would like to change all
the CheckMaps, too, in the process.
On 2011/05/10 13:38:06, Mads Ager wrote:
> Creating an enum for this would make this call site a bit easier to read.
> CHECK_SMI/DONT_CHECK_SMI?

http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:3190: KeyedStoreIC keyed_store_ic(isolate());
On 2011/05/10 13:38:06, Mads Ager wrote:
> I would prefer passing in the actual stub here and dealing with allocation
> failures at the higher level at the caller. Creating KeyedStoreICs outside of
> ic.cc seems a bit fishy to me.
> 
> Same comment for the other occurrences. :-)

Done.

http://codereview.chromium.org/6894003/diff/35002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:3785: __ ldr(r3, FieldMemOperand(receiver,
JSObject::kElementsOffset));
Yes, this stub is always tail-called by other stubs that guarantee "non
smi-ness". Added comments to that effect. 
On 2011/05/10 13:38:06, Mads Ager wrote:
> Have you always checked that the receiver is not a smi when you get here?

http://codereview.chromium.org/6894003/diff/35002/src/code-stubs.h
File src/code-stubs.h (right):

http://codereview.chromium.org/6894003/diff/35002/src/code-stubs.h#newcode993
src/code-stubs.h:993: class KeyedStoreStub : public CodeStub {
Yes, that's a really good idea. Should have thought of it myself. Done.
On 2011/05/10 13:38:06, Mads Ager wrote:
> Why not just have this one and have a minor key for Object and one for Array?
> They share code generation.

http://codereview.chromium.org/6894003/diff/35002/src/code-stubs.h#newcode1037
src/code-stubs.h:1037: Major MajorKey() {
On 2011/05/10 13:38:06, Mads Ager wrote:
> Just don't implement Major and Minor key?

Done.

http://codereview.chromium.org/6894003/diff/35002/src/code-stubs.h#newcode1069
src/code-stubs.h:1069: #define DECLARE_EXTERNAL_ARRAY_STUB(op, type)            
              \
On 2011/05/10 13:38:06, Mads Ager wrote:
> Do you really need all of these different types here. They share code
> generation, right? So, why not just have ExternalArrayLoadStub with a minor
key
> that is the type (byte, unsigned, ...)?

Done.

http://codereview.chromium.org/6894003/diff/35002/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/6894003/diff/35002/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:718: Builtins::kKeyedLoadIC_MissForceGeneric);
On 2011/05/10 13:38:06, Mads Ager wrote:
> four-space indent?

Done.

http://codereview.chromium.org/6894003/diff/35002/src/ia32/stub-cache-ia32.cc...
src/ia32/stub-cache-ia32.cc:3641: NearLabel done;
On 2011/05/10 13:38:06, Mads Ager wrote:
> Accidental edit? There are more than this one I think.

Done.

http://codereview.chromium.org/6894003/diff/35002/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6894003/diff/35002/src/ic.cc#newcode1070
src/ic.cc:1070: if (is_js_array) {
On 2011/05/10 13:38:06, Mads Ager wrote:
> If you simplify the stub hierarchy this can be just:
> 
> KeyedLoadStub stub(is_js_array);
> return stub.TryGetCode();

Done.

http://codereview.chromium.org/6894003/diff/35002/src/ic.cc#newcode1082
src/ic.cc:1082: switch (array_type) {
On 2011/05/10 13:38:06, Mads Ager wrote:
> ExternalArrayLoadStub stub(array_type);
> return stub.TryGetCode();
> 
> ?

Done.

http://codereview.chromium.org/6894003/diff/35002/src/ic.cc#newcode1656
src/ic.cc:1656: // the MONOMORPHIC stubs to jump into for all of the receiver
types that it
On 2011/05/10 13:38:06, Mads Ager wrote:
> Is this comment correct any more? You might need the monomorphic stub for the
> typed arrays to get the external array type, but these are not the stubs that
> you use to compose the actual megamorphic stubs, right?

Done.

http://codereview.chromium.org/6894003/diff/35002/src/ic.cc#newcode1692
src/ic.cc:1692: Object* maybe_cached_stub =
receiver->map()->FindInCodeCache(megamorphic_name,
Yes, we should have a global cache instead of caching on the map. As discussed,
I am adding a comment and TODO. 
On 2011/05/10 13:38:06, Mads Ager wrote:
> I'm not sure I understand why you cache this stub on the current receiver's
map.
> It seems to me that this can create a rather strange mix of types that are
> unrelated to this load/store site? I'm concerned that the type feedback
> generated here will be too polymorphic.
> 
> It seems to me that if we want to cache these stubs we should have some sort
of
> global stub cache instead of caching on one of the maps supported by the stub.
> It would be nice to be able to share these by having a mapping from a list of
> maps to a stub handling them.

http://codereview.chromium.org/6894003/diff/35002/src/ic.cc#newcode1811
src/ic.cc:1811: MaybeObject* maybe_update = receiver->UpdateMapCodeCache(name,
result);
Done for the Monomorphic stubs. The megamorphic stubs should use a different
cache anyway as discussed, so I've created a bug and annotated the code.
On 2011/05/10 13:38:06, Mads Ager wrote:
> It seems to me that something like this should be on the stub cache. In
> particular, we need to make sure that the profiler knows about all generated
> code. Not clear to me if it does? It doesn't follow the normal pattern at
least,
> but maybe it can't?

http://codereview.chromium.org/6894003/diff/35002/src/ic.h
File src/ic.h (right):

http://codereview.chromium.org/6894003/diff/35002/src/ic.h#newcode337
src/ic.h:337: virtual MaybeObject* ComputeMonomorphicBuiltinFastElementStub(
On 2011/05/10 13:38:06, Mads Ager wrote:
> Remove the Builtin from the name since it is not a builtin but a normal stub?

Done.

[Lasse Reichstein, 2011-05-12 08:33:45]

drive-by comment.

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

http://codereview.chromium.org/6894003/diff/43002/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:2538: Handle<Code>(handler_ics->at(current)),
You repeatedly read the map out of the value. It'll obviously be hot, so the
memory compare should be quick. Still, would it be possible to find a scratch
register and read the map once, before the checks, and only do register
compares? 
It would save one byte per compare, at the cost of one memory load (~4 bytes
IIRC).

[Mads Ager (chromium), 2011-05-12 09:42:47]

LGTM, with a couple of nits.

http://codereview.chromium.org/6894003/diff/43002/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/6894003/diff/43002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:3128: true);
I already forgot what this boolean is. Using an enum value would make it easier
to read.

http://codereview.chromium.org/6894003/diff/43002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4114: __ tst(r0, Operand(kSmiTagMask));
Use JumpIfNotSmi?

http://codereview.chromium.org/6894003/diff/43002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4166: __ tst(key_reg, Operand(kSmiTagMask));
JumpIfNotSmi?

http://codereview.chromium.org/6894003/diff/43002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4172: __ ldr(scratch, FieldMemOperand(elements_reg,
HeapObject::kMapOffset));
Can you use CheckMap with kFixedArrayMapRootIndex here?

http://codereview.chromium.org/6894003/diff/43002/src/code-stubs.h
File src/code-stubs.h (right):

http://codereview.chromium.org/6894003/diff/43002/src/code-stubs.h#newcode950
src/code-stubs.h:950: bool is_js_array_;
Remove, this is unused?

http://codereview.chromium.org/6894003/diff/43002/src/code-stubs.h#newcode960
src/code-stubs.h:960: int MinorKey() { return is_js_array_; }
Let's do an explicit type conversion here:

return is_js_array ? 1 : 0;

http://codereview.chromium.org/6894003/diff/43002/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6894003/diff/43002/src/ic.cc#newcode1590
src/ic.cc:1590: // cases (external arrays need the array type from the
MONOMORPHIC stub).
I don't know how much that will happen, but we shouldn't generate the
monomorphic fast elements stub unless we need them. So we shouldn't generate
them if we are (going) megamorphic. They are not needed later in that case.

That should be simple to avoid?

http://codereview.chromium.org/6894003/diff/43002/src/ic.h
File src/ic.h (right):

http://codereview.chromium.org/6894003/diff/43002/src/ic.h#newcode329
src/ic.h:329: 
Could you use two blank lines between class declarations?

http://codereview.chromium.org/6894003/diff/43002/src/ic.h#newcode375
src/ic.h:375: 
Ditto.

http://codereview.chromium.org/6894003/diff/43002/src/log.h
File src/log.h (right):

http://codereview.chromium.org/6894003/diff/43002/src/log.h#newcode125
src/log.h:125: V(KEYED_LOAD_MEGAMORPHIC_IC_TAG,  "KeyedLoadMegamorphicIC")   \
Not introduced with your change, but can you right align all of the '\' here?

http://codereview.chromium.org/6894003/diff/43002/src/objects.cc
File src/objects.cc (left):

http://codereview.chromium.org/6894003/diff/43002/src/objects.cc#oldcode3767
src/objects.cc:3767: ASSERT(code->ic_state() == MONOMORPHIC);
We should reintroduce this assert once we have removed the megamorphic keyed ICs
from the map caches.

http://codereview.chromium.org/6894003/diff/43002/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/6894003/diff/43002/src/objects.h#newcode6712
src/objects.h:6712: class Map;
Can we move these to list.h and avoid depending on list.h here? It seems that
forward declarations in list.h should be enough?

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

http://codereview.chromium.org/6894003/diff/43002/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:3546: masm->isolate()->factory()->fixed_array_map());
You could load this from the root array into kScratchRegister and compare to
that instead of embedding the map here.

[danno, 2011-06-01 13:16:08]

Feedback addressed and landed.

http://codereview.chromium.org/6894003/diff/43002/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/6894003/diff/43002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:3128: true);
On 2011/05/12 09:42:47, Mads Ager wrote:
> I already forgot what this boolean is. Using an enum value would make it
easier
> to read.

Done.

http://codereview.chromium.org/6894003/diff/43002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4114: __ tst(r0, Operand(kSmiTagMask));
On 2011/05/12 09:42:47, Mads Ager wrote:
> Use JumpIfNotSmi?

Done.

http://codereview.chromium.org/6894003/diff/43002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4166: __ tst(key_reg, Operand(kSmiTagMask));
On 2011/05/12 09:42:47, Mads Ager wrote:
> JumpIfNotSmi?

Done.

http://codereview.chromium.org/6894003/diff/43002/src/arm/stub-cache-arm.cc#n...
src/arm/stub-cache-arm.cc:4172: __ ldr(scratch, FieldMemOperand(elements_reg,
HeapObject::kMapOffset));
On 2011/05/12 09:42:47, Mads Ager wrote:
> Can you use CheckMap with kFixedArrayMapRootIndex here?

Done.

http://codereview.chromium.org/6894003/diff/43002/src/code-stubs.h
File src/code-stubs.h (right):

http://codereview.chromium.org/6894003/diff/43002/src/code-stubs.h#newcode950
src/code-stubs.h:950: bool is_js_array_;
On 2011/05/12 09:42:47, Mads Ager wrote:
> Remove, this is unused?

Done.

http://codereview.chromium.org/6894003/diff/43002/src/code-stubs.h#newcode960
src/code-stubs.h:960: int MinorKey() { return is_js_array_; }
On 2011/05/12 09:42:47, Mads Ager wrote:
> Let's do an explicit type conversion here:
> 
> return is_js_array ? 1 : 0;

Done.

http://codereview.chromium.org/6894003/diff/43002/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/6894003/diff/43002/src/ic.cc#newcode1590
src/ic.cc:1590: // cases (external arrays need the array type from the
MONOMORPHIC stub).
On 2011/05/12 09:42:47, Mads Ager wrote:
> I don't know how much that will happen, but we shouldn't generate the
> monomorphic fast elements stub unless we need them. So we shouldn't generate
> them if we are (going) megamorphic. They are not needed later in that case.
> 
> That should be simple to avoid?

It's not the easy here, unfortunately. We need to make sure that the array type
is available from the monomorphic stub, otherwise we don't know which external
array shared stub to use in the megamorphic case (thus the comment). I'll add a
TODO that when we finally figure out a better way to track the external array
type with the map that we should remove this.

http://codereview.chromium.org/6894003/diff/43002/src/ic.h
File src/ic.h (right):

http://codereview.chromium.org/6894003/diff/43002/src/ic.h#newcode329
src/ic.h:329: 
On 2011/05/12 09:42:47, Mads Ager wrote:
> Could you use two blank lines between class declarations?

Done.

http://codereview.chromium.org/6894003/diff/43002/src/ic.h#newcode375
src/ic.h:375: 
On 2011/05/12 09:42:47, Mads Ager wrote:
> Ditto.

Done.

http://codereview.chromium.org/6894003/diff/43002/src/log.h
File src/log.h (right):

http://codereview.chromium.org/6894003/diff/43002/src/log.h#newcode125
src/log.h:125: V(KEYED_LOAD_MEGAMORPHIC_IC_TAG,  "KeyedLoadMegamorphicIC")   \
On 2011/05/12 09:42:47, Mads Ager wrote:
> Not introduced with your change, but can you right align all of the '\' here?

Done.

http://codereview.chromium.org/6894003/diff/43002/src/objects.cc
File src/objects.cc (left):

http://codereview.chromium.org/6894003/diff/43002/src/objects.cc#oldcode3767
src/objects.cc:3767: ASSERT(code->ic_state() == MONOMORPHIC);
Agreed.
On 2011/05/12 09:42:47, Mads Ager wrote:
> We should reintroduce this assert once we have removed the megamorphic keyed
ICs
> from the map caches.

http://codereview.chromium.org/6894003/diff/43002/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/6894003/diff/43002/src/objects.h#newcode6712
src/objects.h:6712: class Map;
On 2011/05/12 09:42:47, Mads Ager wrote:
> Can we move these to list.h and avoid depending on list.h here? It seems that
> forward declarations in list.h should be enough?

Done.

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

http://codereview.chromium.org/6894003/diff/43002/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:2538: Handle<Code>(handler_ics->at(current)),
On 2011/05/12 08:33:46, Lasse Reichstein wrote:
> You repeatedly read the map out of the value. It'll obviously be hot, so the
> memory compare should be quick. Still, would it be possible to find a scratch
> register and read the map once, before the checks, and only do register
> compares? 
> It would save one byte per compare, at the cost of one memory load (~4 bytes
> IIRC).

Done.

http://codereview.chromium.org/6894003/diff/43002/src/x64/stub-cache-x64.cc#n...
src/x64/stub-cache-x64.cc:3546: masm->isolate()->factory()->fixed_array_map());
On 2011/05/12 09:42:47, Mads Ager wrote:
> You could load this from the root array into kScratchRegister and compare to
> that instead of embedding the map here.

Done.