This change allows generating call-stubs for objects with normal (non-fast) o...

This change allows generating call-stubs for objects with normal (non-fast) objects in the prototype chain. StubCompiler::CheckPrototypes does ne

If the top level object is a normal object the stub is stored in its prototype map.

Lookup result of type NORMAL is not covered (since the normal stub currently doesn't check the prototype chain).

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

[Mads Ager (chromium), 2010-06-28 11:40:08]

I would like to restrict this a bit at first. I don't think we want to generate
ICs for all combinations of slow and fast case objects. The stubs will be huge
and it will not be clear where to cache them. In order to keep this small and
isolated at first I would like to start out with only doing this for a slow-case
receiver with the property on a fast case prototype.

First batch of comments included.

http://codereview.chromium.org/2801018/diff/39001/9002
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/2801018/diff/39001/9002#newcode745
src/arm/stub-cache-arm.cc:745: Register) {
Let's move this register argument to before/after scratch and give it a name?

http://codereview.chromium.org/2801018/diff/39001/9003
File src/globals.h (right):

http://codereview.chromium.org/2801018/diff/39001/9003#newcode468
src/globals.h:468: PROTOTYPE_MAP  // For hashes (except GlobalObjects).
hashes -> slow properties objects?

http://codereview.chromium.org/2801018/diff/39001/9006
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/2801018/diff/39001/9006#newcode154
src/ia32/stub-cache-ia32.cc:154: static const int kProbes = 4;
This loop has to be different. We need to check for deleted elements in the
dictionary. We need to skip deleted elements until we reach an element that is
not deleted. At that point, if the element is absent we can proceed.

http://codereview.chromium.org/2801018/diff/39001/9006#newcode858
src/ia32/stub-cache-ia32.cc:858: // fast and global obgects or do negative
lookup for normal objects.
obgects -> objects

http://codereview.chromium.org/2801018/diff/39001/9007
File src/ic-inl.h (right):

http://codereview.chromium.org/2801018/diff/39001/9007#newcode97
src/ic-inl.h:97: !object->HasFastProperties() &&
I don't think this is what we want to do. I don't think we want to use the
prototype map for all stubs generated for objects that do not have fast
properties. I think we should only use the prototype map for the new call stubs.

http://codereview.chromium.org/2801018/diff/39001/9008
File src/ic.cc (right):

http://codereview.chromium.org/2801018/diff/39001/9008#newcode138
src/ic.cc:138: // Returns false when generating a stub is possible, but effect
is usually
I don't understand why these conditions are right? Please explain why these
conditions make sense?

http://codereview.chromium.org/2801018/diff/39001/9008#newcode140
src/ic.cc:140: static bool DoesWorthToCache(LookupResult* lookup, Object*
receiver) {
DoesWorthToCache -> IsWorthCaching

http://codereview.chromium.org/2801018/diff/39001/9008#newcode150
src/ic.cc:150: static bool HashNormalObjectsInPrototypeChain(LookupResult*
lookup,
Hash -> Has?

http://codereview.chromium.org/2801018/diff/39001/9008#newcode177
src/ic.cc:177: if (cache_holder == OWN_MAP &&
I don't understand why you have to change anything for the case where the cache
holder is the map of the object itself. We should only use the prototype map for
the newly added call stubs?

This code seems to complicated. We should be able to keep this as is except for
the new type of IC where we have to do something different.

http://codereview.chromium.org/2801018/diff/39001/9008#newcode183
src/ic.cc:183: // or a global object. IC::GetCodeCacheMap is not appliable.
appliable -> applicable

http://codereview.chromium.org/2801018/diff/39001/9008#newcode187
src/ic.cc:187: // IC::GetCodeCacheMap is not appliable.
appliable -> applicable

http://codereview.chromium.org/2801018/diff/39001/9008#newcode645
src/ic.cc:645: // Cache code holding map should be consistent with
I don't understand the need for this change. You only need to change how this
works for the new type of stub. That new type of stub has special code flags so
you can figure out which map to use.

http://codereview.chromium.org/2801018/diff/39001/9009
File src/ic.h (right):

http://codereview.chromium.org/2801018/diff/39001/9009#newcode120
src/ic.h:120: // Determines which map must be used for keeping the code stub.
I don't think this is the right place to determine. I think we should leave
GetCodeCacheMapForObject intact and split this in the IC LoadFunction code
instead.

http://codereview.chromium.org/2801018/diff/39001/9010
File src/objects-inl.h (right):

http://codereview.chromium.org/2801018/diff/39001/9010#newcode2274
src/objects-inl.h:2274: if (holder == PROTOTYPE_MAP) bits |= kFlagsICHolderMask;
kFlagsCacheInPrototypeMapMask?

[SeRya, 2010-06-29 13:24:31]

I chancged the code to allow the new call-stubs for in the direct prototype of a
slow properties object only.

http://codereview.chromium.org/2801018/diff/39001/9002
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/2801018/diff/39001/9002#newcode745
src/arm/stub-cache-arm.cc:745: Register) {
On 2010/06/28 11:40:08, Mads Ager wrote:
> Let's move this register argument to before/after scratch and give it a name?

I added this argument as a last one since is is not used in the most cases (at
least now, while this chenche only exists for ia32). It allows to keep unchanged
~45 of ~58 usings of CheckPrototypes. Do you really think it make sense to move
this parameter?

http://codereview.chromium.org/2801018/diff/39001/9003
File src/globals.h (right):

http://codereview.chromium.org/2801018/diff/39001/9003#newcode468
src/globals.h:468: PROTOTYPE_MAP  // For hashes (except GlobalObjects).
On 2010/06/28 11:40:08, Mads Ager wrote:
> hashes -> slow properties objects?

Done.

http://codereview.chromium.org/2801018/diff/39001/9006
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/2801018/diff/39001/9006#newcode154
src/ia32/stub-cache-ia32.cc:154: static const int kProbes = 4;
On 2010/06/28 11:40:08, Mads Ager wrote:
> This loop has to be different. We need to check for deleted elements in the
> dictionary. We need to skip deleted elements until we reach an element that is
> not deleted. At that point, if the element is absent we can proceed.

This loop intentionally doesn't check for deleted properties. The check may
decrease false-negative rate, but would cost two additional instructions per
iteration. At least I'd prefer to add it in a separate CL if an experiment has
shown that it makes sense.

http://codereview.chromium.org/2801018/diff/39001/9006#newcode858
src/ia32/stub-cache-ia32.cc:858: // fast and global obgects or do negative
lookup for normal objects.
On 2010/06/28 11:40:08, Mads Ager wrote:
> obgects -> objects
> 

Done.

http://codereview.chromium.org/2801018/diff/39001/9007
File src/ic-inl.h (right):

http://codereview.chromium.org/2801018/diff/39001/9007#newcode97
src/ic-inl.h:97: !object->HasFastProperties() &&
On 2010/06/28 11:40:08, Mads Ager wrote:
> I don't think this is what we want to do. I don't think we want to use the
> prototype map for all stubs generated for objects that do not have fast
> properties. I think we should only use the prototype map for the new call
stubs.

This code DOES return PROTOTYPE_MAP for the new stubs only (slow properties
objects except globals what don't contain the property). Since we do not
generate load stubs such objects they are call stubs.

http://codereview.chromium.org/2801018/diff/39001/9008
File src/ic.cc (right):

http://codereview.chromium.org/2801018/diff/39001/9008#newcode138
src/ic.cc:138: // Returns false when generating a stub is possible, but effect
is usually
On 2010/06/28 11:40:08, Mads Ager wrote:
> I don't understand why these conditions are right? Please explain why these
> conditions make sense?

The idea was to not generate new load stubs. This code don't check for slow
objects in the middle considering it as a rare case. So I checked the first
object (except it's a holder or property is not found: both cases have special
stubs).

Anyway making conditions stricter for now I remover this function using
HasNormalObjectsInPrototypeChain instead.

http://codereview.chromium.org/2801018/diff/39001/9008#newcode140
src/ic.cc:140: static bool DoesWorthToCache(LookupResult* lookup, Object*
receiver) {
On 2010/06/28 11:40:08, Mads Ager wrote:
> DoesWorthToCache -> IsWorthCaching

Done.

http://codereview.chromium.org/2801018/diff/39001/9008#newcode150
src/ic.cc:150: static bool HashNormalObjectsInPrototypeChain(LookupResult*
lookup,
On 2010/06/28 11:40:08, Mads Ager wrote:
> Hash -> Has?

Done.

http://codereview.chromium.org/2801018/diff/39001/9008#newcode177
src/ic.cc:177: if (cache_holder == OWN_MAP &&
On 2010/06/28 11:40:08, Mads Ager wrote:
> I don't understand why you have to change anything for the case where the
cache
> holder is the map of the object itself. We should only use the prototype map
for
> the newly added call stubs?
> 
> This code seems to complicated. We should be able to keep this as is except
for
> the new type of IC where we have to do something different.

The condition was complicated in order to recognize cases where IndexInCodeCache
could be avoided. But you are right, this optimization would be hard to
maintain. I left only essential check.

http://codereview.chromium.org/2801018/diff/39001/9008#newcode183
src/ic.cc:183: // or a global object. IC::GetCodeCacheMap is not appliable.
On 2010/06/28 11:40:08, Mads Ager wrote:
> appliable -> applicable

Done.

http://codereview.chromium.org/2801018/diff/39001/9008#newcode187
src/ic.cc:187: // IC::GetCodeCacheMap is not appliable.
On 2010/06/28 11:40:08, Mads Ager wrote:
> appliable -> applicable

Done.

http://codereview.chromium.org/2801018/diff/39001/9008#newcode645
src/ic.cc:645: // Cache code holding map should be consistent with
On 2010/06/28 11:40:08, Mads Ager wrote:
> I don't understand the need for this change. You only need to change how this
> works for the new type of stub. That new type of stub has special code flags
so
> you can figure out which map to use.

It is not the same maps. This map is used to calculate a hash code for the stub
cache table. Here show properties objects use their own map (in sprite such a
stub is stored in prototype's map) to avoid handling special case in
GenerateMonomorphicCacheProbe.

http://codereview.chromium.org/2801018/diff/39001/9009
File src/ic.h (right):

http://codereview.chromium.org/2801018/diff/39001/9009#newcode120
src/ic.h:120: // Determines which map must be used for keeping the code stub.
On 2010/06/28 11:40:08, Mads Ager wrote:
> I don't think this is the right place to determine. I think we should leave
> GetCodeCacheMapForObject intact and split this in the IC LoadFunction code
> instead.

I didn't get it. I see no sense putting this code into CallICBase::LoadFunction.
It could be placed into CallICBase::UpdateCaches and the result could be passed
as an additional parameter into ComputeCall*. Did you mean that?

http://codereview.chromium.org/2801018/diff/39001/9010
File src/objects-inl.h (right):

http://codereview.chromium.org/2801018/diff/39001/9010#newcode2274
src/objects-inl.h:2274: if (holder == PROTOTYPE_MAP) bits |= kFlagsICHolderMask;
On 2010/06/28 11:40:08, Mads Ager wrote:
> kFlagsCacheInPrototypeMapMask?

Done.

[Mads Ager (chromium), 2010-06-30 12:10:16]

Next round of comments. I think there is still room for simplifications but we
are definitely getting closer.

http://codereview.chromium.org/2801018/diff/10003/29008
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/2801018/diff/10003/29008#newcode108
src/ia32/stub-cache-ia32.cc:108: // Name must be a symbol.
and receiver must be a heap object.

http://codereview.chromium.org/2801018/diff/10003/29008#newcode123
src/ia32/stub-cache-ia32.cc:123: const int bit_flags = (1 <<
Map::kHasNamedInterceptor) |
bit_flags -> kInterceptorOrAccessCheckNeededMask

or something similar.

http://codereview.chromium.org/2801018/diff/10003/29008#newcode125
src/ia32/stub-cache-ia32.cc:125: // Bail out if the receiver has a named
interceptor.
or requires access checks.

http://codereview.chromium.org/2801018/diff/10003/29008#newcode132
src/ia32/stub-cache-ia32.cc:132: if (check_object_type) {
I don't understand this part. You have just checked that r0 is JS_OBJECT_TYPE.
If it is not, you bail out. Now you are doing another conditional instance type
check. Which of them do you want?

http://codereview.chromium.org/2801018/diff/10003/29008#newcode178
src/ia32/stub-cache-ia32.cc:178: // Having undefined at thins place means the
name is not contained.
thins -> this

You should be more explicit in this comment. I now understand why you don't have
to explictly check for deleted elements here because we use null for delete
elements instead of undefined, so check for undefined you are checking both for
existing and for deleted elements. Could you expand the comment to be explicit
about that?

http://codereview.chromium.org/2801018/diff/10003/29008#newcode190
src/ia32/stub-cache-ia32.cc:190: ASSERT(!Heap::InNewSpace(name));
Not necessary. You have already asserted that name is a symbol.

http://codereview.chromium.org/2801018/diff/10003/29008#newcode845
src/ia32/stub-cache-ia32.cc:845: // Make sure there's no overlap between scratch
and the other
I think this can be simplified because we only allow the receiver to be a
dictionary case object. I think we can keep this code as it was except that we
check for slow properties on the object first. If the object has slow properties
we call GenerateNegativeDictionaryLookup and use CheckMaps with
object->GetPrototype().

I would like to get this in as a minimal delta to what is there already so it is
easy to follow.

For the rest of the chain we can assert that they have fast properties.

http://codereview.chromium.org/2801018/diff/10003/29008#newcode871
src/ia32/stub-cache-ia32.cc:871: if (!name->IsSymbol()) {
When do we get here where name is not a symbol? Could this be an assert that
name is a symbol? For normal call ICs the name will be a symbol. For keyed call
ICs I would think that a symbol check takes place before the prototype check?

Can we make this an assert that they name is a symbol?

http://codereview.chromium.org/2801018/diff/10003/29009
File src/ic-inl.h (right):

http://codereview.chromium.org/2801018/diff/10003/29009#newcode96
src/ic-inl.h:96: if (holder != object &&
Please add a comment explaining this condition.

http://codereview.chromium.org/2801018/diff/10003/29010
File src/ic.cc (right):

http://codereview.chromium.org/2801018/diff/10003/29010#newcode139
src/ic.cc:139: Object* receiver) {
indentation

http://codereview.chromium.org/2801018/diff/10003/29010#newcode141
src/ic.cc:141: for (Object* i = receiver; i != end; i = i->GetPrototype()) {
i -> current?

http://codereview.chromium.org/2801018/diff/10003/29010#newcode520
src/ic.cc:520: State state,
Indentation, not your fault, but could you fix it? :)

http://codereview.chromium.org/2801018/diff/10003/29010#newcode530
src/ic.cc:530: if (lookup->holder() != object->GetPrototype() &&
I'm not sure this condition is the right one? If the holder is not the receiver
we should check that there are no normal objects starting from the prototype.

The current check would disallow stuff like:

normal receiver -proto-> fast properties -proto-> function here on fast
properties object

?

http://codereview.chromium.org/2801018/diff/10003/29011
File src/ic.h (right):

http://codereview.chromium.org/2801018/diff/10003/29011#newcode121
src/ic.h:121: // This methods should not be called with undefined or null.
This methods -> These methods

http://codereview.chromium.org/2801018/diff/10003/29015
File src/stub-cache.cc (right):

http://codereview.chromium.org/2801018/diff/10003/29015#newcode406
src/stub-cache.cc:406: PROFILE(CodeCreateEvent(Logger::STORE_IC_TAG,
Code::cast(code), name));
Good catch, thanks. :)

[SeRya, 2010-06-30 16:52:14]

http://codereview.chromium.org/2801018/diff/10003/29008
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/2801018/diff/10003/29008#newcode108
src/ia32/stub-cache-ia32.cc:108: // Name must be a symbol.
On 2010/06/30 12:10:16, Mads Ager wrote:
> and receiver must be a heap object.

Done.

http://codereview.chromium.org/2801018/diff/10003/29008#newcode123
src/ia32/stub-cache-ia32.cc:123: const int bit_flags = (1 <<
Map::kHasNamedInterceptor) |
On 2010/06/30 12:10:16, Mads Ager wrote:
> bit_flags -> kInterceptorOrAccessCheckNeededMask
> 
> or something similar.

Done.

http://codereview.chromium.org/2801018/diff/10003/29008#newcode125
src/ia32/stub-cache-ia32.cc:125: // Bail out if the receiver has a named
interceptor.
On 2010/06/30 12:10:16, Mads Ager wrote:
> or requires access checks.

Done.

http://codereview.chromium.org/2801018/diff/10003/29008#newcode132
src/ia32/stub-cache-ia32.cc:132: if (check_object_type) {
On 2010/06/30 12:10:16, Mads Ager wrote:
> I don't understand this part. You have just checked that r0 is JS_OBJECT_TYPE.
> If it is not, you bail out. Now you are doing another conditional instance
type
> check. Which of them do you want?

Removed. The first object in the chain could be not-null and not-js, but I
didn't spotted that the checking code is the same.

http://codereview.chromium.org/2801018/diff/10003/29008#newcode178
src/ia32/stub-cache-ia32.cc:178: // Having undefined at thins place means the
name is not contained.
On 2010/06/30 12:10:16, Mads Ager wrote:
> thins -> this
> 
> You should be more explicit in this comment. I now understand why you don't
have
> to explictly check for deleted elements here because we use null for delete
> elements instead of undefined, so check for undefined you are checking both
for
> existing and for deleted elements. Could you expand the comment to be explicit
> about that?

Done.

http://codereview.chromium.org/2801018/diff/10003/29008#newcode190
src/ia32/stub-cache-ia32.cc:190: ASSERT(!Heap::InNewSpace(name));
On 2010/06/30 12:10:16, Mads Ager wrote:
> Not necessary. You have already asserted that name is a symbol.

Done.

http://codereview.chromium.org/2801018/diff/10003/29008#newcode845
src/ia32/stub-cache-ia32.cc:845: // Make sure there's no overlap between scratch
and the other
On 2010/06/30 12:10:16, Mads Ager wrote:
> I think this can be simplified because we only allow the receiver to be a
> dictionary case object. I think we can keep this code as it was except that we
> check for slow properties on the object first. If the object has slow
properties
> we call <div style="display: inline; height: auto; position: absolute;
visibility: hidden; width: auto;
">GenerateNegativeDictionaryLookup</div>GenerateNegativeDictionaryLookup and use
CheckMaps with
> object->GetPrototype().
> 
> I would like to get this in as a minimal delta to what is there already so it
is
> easy to follow.
> 
> For the rest of the chain we can assert that they have fast properties.

You actually suggest to move a piece of code out of the loop. I don't think it's
simplification. It would make the code less general giving nothing back (what
sense to propagate the decision to not allow slow objects in the middle through
the codebase?)

If the point is to reduce the delta I would suggest restore the CheckMaps
function, add the 'name' paramater and move the new code there (together with
GenerateNegativeLookup). However I dislike it because: 1) CheckMaps is used only
once 2) CheckPrototype represent almost the same level of abstraction 3) it
multiplies entities unreasonably.

http://codereview.chromium.org/2801018/diff/10003/29008#newcode871
src/ia32/stub-cache-ia32.cc:871: if (!name->IsSymbol()) {
On 2010/06/30 12:10:16, Mads Ager wrote:
> When do we get here where name is not a symbol? Could this be an assert that
> name is a symbol? For normal call ICs the name will be a symbol. For keyed
call
> ICs I would think that a symbol check takes place before the prototype check?
> 
> Can we make this an assert that they name is a symbol?

CheckPrototypes is definitely called with non-symbol value (I added such an
assert at the begin of the method and it broke a lot of tests). That calls do
not cause negative lookups but I think it's much safer to be ready for this
case.

http://codereview.chromium.org/2801018/diff/10003/29009
File src/ic-inl.h (right):

http://codereview.chromium.org/2801018/diff/10003/29009#newcode96
src/ic-inl.h:96: if (holder != object &&
On 2010/06/30 12:10:16, Mads Ager wrote:
> Please add a comment explaining this condition.

Done.

http://codereview.chromium.org/2801018/diff/10003/29010
File src/ic.cc (right):

http://codereview.chromium.org/2801018/diff/10003/29010#newcode139
src/ic.cc:139: Object* receiver) {
On 2010/06/30 12:10:16, Mads Ager wrote:
> indentation

Done.

http://codereview.chromium.org/2801018/diff/10003/29010#newcode141
src/ic.cc:141: for (Object* i = receiver; i != end; i = i->GetPrototype()) {
On 2010/06/30 12:10:16, Mads Ager wrote:
> i -> current?

Done.

http://codereview.chromium.org/2801018/diff/10003/29010#newcode520
src/ic.cc:520: State state,
On 2010/06/30 12:10:16, Mads Ager wrote:
> Indentation, not your fault, but could you fix it? :)

Done.

http://codereview.chromium.org/2801018/diff/10003/29010#newcode530
src/ic.cc:530: if (lookup->holder() != object->GetPrototype() &&
On 2010/06/30 12:10:16, Mads Ager wrote:
> I'm not sure this condition is the right one? If the holder is not the
receiver
> we should check that there are no normal objects starting from the prototype.
> 
> The current check would disallow stuff like:
> 
> normal receiver -proto-> fast properties -proto-> function here on fast
> properties object
> 
> ?
> 

Done.

http://codereview.chromium.org/2801018/diff/10003/29011
File src/ic.h (right):

http://codereview.chromium.org/2801018/diff/10003/29011#newcode121
src/ic.h:121: // This methods should not be called with undefined or null.
On 2010/06/30 12:10:16, Mads Ager wrote:
> This methods -> These methods

Done.

[Mads Ager (chromium), 2010-07-01 10:11:29]

A few more comments.

http://codereview.chromium.org/2801018/diff/54003/66007
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/2801018/diff/54003/66007#newcode104
src/ia32/stub-cache-ia32.cc:104: // Helper function used to check that the
dictionary doesn't contain the prop.
prop -> property.

http://codereview.chromium.org/2801018/diff/54003/66007#newcode172
src/ia32/stub-cache-ia32.cc:172: // Having undefined at this place means the
name is not contained.
Please expand this comment to state that the undefined check takes care of both
present and deleted elements because deleted elements use null as the name.

http://codereview.chromium.org/2801018/diff/54003/66007#newcode177
src/ia32/stub-cache-ia32.cc:177: if (entity_name.is(properties)) {
Could you use extra.is(no_reg) for consistency with the condition for pushing
the receiver?

http://codereview.chromium.org/2801018/diff/54003/66007#newcode187
src/ia32/stub-cache-ia32.cc:187: if (entity_name.is(properties)) {
Could you use extra.is(no_reg) here as well?

http://codereview.chromium.org/2801018/diff/54003/66007#newcode828
src/ia32/stub-cache-ia32.cc:828: Register
StubCompiler::CheckPrototypes(JSObject* object,
Comments should be added to CheckPrototypes to state what it does. Folding
CheckMaps into CheckPrototypes makes sense, but we should document what
CheckPrototypes does.

Please document register use, push_at_depth, and what can be expected as a
result of calling CheckPrototypes. Starting from the CheckMaps comment that has
now been removed would make sense.

http://codereview.chromium.org/2801018/diff/54003/66007#newcode839
src/ia32/stub-cache-ia32.cc:839: // registers.
other registers -> holder and object registers.

You do not check extra and you probably don't have to?

http://codereview.chromium.org/2801018/diff/54003/66007#newcode930
src/ia32/stub-cache-ia32.cc:930: // Check that the object layout has not changed
(if it's a fast properties
Could you go back to the original "Check the holder map." comment there. I don't
find the comment that you have now useful. The map check implies a lot of things
and not just for properties of type CONSTANT_FUNCTION. It does not imply that
the object is the same.

http://codereview.chromium.org/2801018/diff/54003/66007#newcode940
src/ia32/stub-cache-ia32.cc:940: // Perform security check for access to the
global object and return
Remove the 'return the holder register part' since you do not do this yet.

http://codereview.chromium.org/2801018/diff/54003/66007#newcode948
src/ia32/stub-cache-ia32.cc:948: current = object;
Please reinstate the comment about having to check for global property cell
equality if we have skipped any global objects.

I still think it would make sense to split this into a couple of helper methods
to make this code easier to read. This method could be something like:

CheckPrototypes:
  if receiver is slow case:
    NegativeDictionaryLookup on receiver
    object = receiver->GetPrototype()
  CheckMaps from object to holder
  CheckGlobalPropertyCells

I would find that much easier to read as the user of this method. Then you can
dive into the details of the individual parts if you want to.

http://codereview.chromium.org/2801018/diff/54003/66008
File src/ic-inl.h (right):

http://codereview.chromium.org/2801018/diff/54003/66008#newcode99
src/ic-inl.h:99: // the same prototype (or a prototypes with the same map) and
not having
or a prototypes -> or a prototype

[SeRya, 2010-07-01 19:21:11]

http://codereview.chromium.org/2801018/diff/54003/66007
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/2801018/diff/54003/66007#newcode104
src/ia32/stub-cache-ia32.cc:104: // Helper function used to check that the
dictionary doesn't contain the prop.
On 2010/07/01 10:11:29, Mads Ager wrote:
> prop -> property.

Done.

http://codereview.chromium.org/2801018/diff/54003/66007#newcode172
src/ia32/stub-cache-ia32.cc:172: // Having undefined at this place means the
name is not contained.
On 2010/07/01 10:11:29, Mads Ager wrote:
> Please expand this comment to state that the undefined check takes care of
both
> present and deleted elements because deleted elements use null as the name.

I woudn't say this is the place which takes care of deleted elements. Instead I
added a comment above describing why this method is correct and why deleted
values don't break it.

http://codereview.chromium.org/2801018/diff/54003/66007#newcode177
src/ia32/stub-cache-ia32.cc:177: if (entity_name.is(properties)) {
On 2010/07/01 10:11:29, Mads Ager wrote:
> Could you use extra.is(no_reg) for consistency with the condition for pushing
> the receiver?

Done.

http://codereview.chromium.org/2801018/diff/54003/66007#newcode187
src/ia32/stub-cache-ia32.cc:187: if (entity_name.is(properties)) {
On 2010/07/01 10:11:29, Mads Ager wrote:
> Could you use extra.is(no_reg) here as well?

Done.

http://codereview.chromium.org/2801018/diff/54003/66007#newcode828
src/ia32/stub-cache-ia32.cc:828: Register
StubCompiler::CheckPrototypes(JSObject* object,
On 2010/07/01 10:11:29, Mads Ager wrote:
> Comments should be added to CheckPrototypes to state what it does. Folding
> CheckMaps into CheckPrototypes makes sense, but we should document what
> CheckPrototypes does.
> 
> Please document register use, push_at_depth, and what can be expected as a
> result of calling CheckPrototypes. Starting from the CheckMaps comment that
has
> now been removed would make sense.

Added comments to stub-cache.h.

http://codereview.chromium.org/2801018/diff/54003/66007#newcode839
src/ia32/stub-cache-ia32.cc:839: // registers.
On 2010/07/01 10:11:29, Mads Ager wrote:
> other registers -> holder and object registers.
> 
> You do not check extra and you probably don't have to?

Done.

http://codereview.chromium.org/2801018/diff/54003/66007#newcode930
src/ia32/stub-cache-ia32.cc:930: // Check that the object layout has not changed
(if it's a fast properties
On 2010/07/01 10:11:29, Mads Ager wrote:
> Could you go back to the original "Check the holder map." comment there. I
don't
> find the comment that you have now useful. The map check implies a lot of
things
> and not just for properties of type CONSTANT_FUNCTION. It does not imply that
> the object is the same.

Done.

http://codereview.chromium.org/2801018/diff/54003/66007#newcode940
src/ia32/stub-cache-ia32.cc:940: // Perform security check for access to the
global object and return
On 2010/07/01 10:11:29, Mads Ager wrote:
> Remove the 'return the holder register part' since you do not do this yet.

Done.

http://codereview.chromium.org/2801018/diff/54003/66007#newcode948
src/ia32/stub-cache-ia32.cc:948: current = object;
On 2010/07/01 10:11:29, Mads Ager wrote:
> Please reinstate the comment about having to check for global property cell
> equality if we have skipped any global objects.

Done.

> I still think it would make sense to split this into a couple of helper
methods
> to make this code easier to read. This method could be something like:
> 
> CheckPrototypes:
>   if receiver is slow case:
>     NegativeDictionaryLookup on receiver
>     object = receiver->GetPrototype()
>   CheckMaps from object to holder
>   CheckGlobalPropertyCells
> 
> I would find that much easier to read as the user of this method. Then you can
> dive into the details of the individual parts if you want to.

I extracted methods GenerateCheckPropertyHolder and GenerateCheckPropertyCells,
but I I left the main loop here.  I still dislike the CheckMaps method (one more
reason I dislike it is the push_at_depth argument). Would be cool to move a
piece of the loop body in to a method but I don't see how to do it keeping its
optimizations. I'l think for a while.

http://codereview.chromium.org/2801018/diff/54003/66008
File src/ic-inl.h (right):

http://codereview.chromium.org/2801018/diff/54003/66008#newcode99
src/ic-inl.h:99: // the same prototype (or a prototypes with the same map) and
not having
On 2010/07/01 10:11:29, Mads Ager wrote:
> or a prototypes -> or a prototype

Done.

[Mads Ager (chromium), 2010-07-02 10:39:41]

LGTM

Address these comments and let's land this. Please port to x64 and arm next so
we can get rid of the ifdef.

http://codereview.chromium.org/2801018/diff/31022/77007
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/2801018/diff/31022/77007#newcode153
src/ia32/stub-cache-ia32.cc:153: // If names of slots in range [0...n - 1) for
the hash value are not
n -> kProbes.

http://codereview.chromium.org/2801018/diff/31022/77007#newcode154
src/ia32/stub-cache-ia32.cc:154: // equal to the name and slot n is not used
(its name is the undefined value),
n -> kProbes - 1?

http://codereview.chromium.org/2801018/diff/31022/77007#newcode831
src/ia32/stub-cache-ia32.cc:831: // [object, holder).
This notation does not make sense for objects. 

for each global object in the prototype chain from object to (but not including)
holder.

?

http://codereview.chromium.org/2801018/diff/31022/77007#newcode858
src/ia32/stub-cache-ia32.cc:858: static void
GenerateCheckPropertyHolder(MacroAssembler* masm,
I don't think this abstraction makes sense and definitely not with this name.
Please inline again or come up with a more descriptive name.

[SeRya, 2010-07-02 13:18:11]

http://codereview.chromium.org/2801018/diff/31022/77007
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/2801018/diff/31022/77007#newcode153
src/ia32/stub-cache-ia32.cc:153: // If names of slots in range [0...n - 1) for
the hash value are not
On 2010/07/02 10:39:41, Mads Ager wrote:
> n -> kProbes.

Done.

http://codereview.chromium.org/2801018/diff/31022/77007#newcode154
src/ia32/stub-cache-ia32.cc:154: // equal to the name and slot n is not used
(its name is the undefined value),
On 2010/07/02 10:39:41, Mads Ager wrote:
> n -> kProbes - 1?

Done.

http://codereview.chromium.org/2801018/diff/31022/77007#newcode831
src/ia32/stub-cache-ia32.cc:831: // [object, holder).
On 2010/07/02 10:39:41, Mads Ager wrote:
> This notation does not make sense for objects. 
> 
> for each global object in the prototype chain from object to (but not
including)
> holder.
> 
> ?

Done.

http://codereview.chromium.org/2801018/diff/31022/77007#newcode858
src/ia32/stub-cache-ia32.cc:858: static void
GenerateCheckPropertyHolder(MacroAssembler* masm,
On 2010/07/02 10:39:41, Mads Ager wrote:
> I don't think this abstraction makes sense and definitely not with this name.
> Please inline again or come up with a more descriptive name.

Done.