Handlify KeyedIC::ComputeStub.

Handlify KeyedIC::ComputeStub.


BUG=
TEST=

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

[ulan, 2011-10-20 14:35:54]

Please take a look.

http://codereview.chromium.org/8356041/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8356041/diff/1/src/ic.cc#newcode1492
src/ic.cc:1492: if (!receiver_maps->at(current).is_null() &&
Unfortunately, receiver_maps can contain nulls (see line 1667).

http://codereview.chromium.org/8356041/diff/1/src/ic.cc#newcode1514
src/ic.cc:1514: for (RelocIterator it(*stub, mask); !it.done(); it.next()) {
Is it possible to handlify the iterator?

http://codereview.chromium.org/8356041/diff/1/src/ic.cc#newcode1667
src/ic.cc:1667: transitioned_maps.Add(transitioned_map);
transitioned_map can be null.

http://codereview.chromium.org/8356041/diff/1/src/stub-cache.cc
File src/stub-cache.cc (right):

http://codereview.chromium.org/8356041/diff/1/src/stub-cache.cc#newcode622
src/stub-cache.cc:622: MapList raw_receiver_maps(receiver_maps->length());
I am not happy with the following 12 lines, but can't think of a better way.

[ulan, 2011-10-20 15:16:17]

http://codereview.chromium.org/8356041/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8356041/diff/1/src/ic.cc#newcode1656
src/ic.cc:1656: cached_stub = FastElementsConversionStub(
Restore ElementsTransitionAndStoreStub, which got deleted during rebase.

[Kevin Millikin (Chromium), 2011-10-21 06:48:13]

Hairy stuff.  LGTM.

I'm not thrilled with the handle list unwrapping, it seems kind of subtle.  Do
you think we'll eventually be able to handlify the polymorphic code cache
directly, so the unwrapping is not necessary?

http://codereview.chromium.org/8356041/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8356041/diff/1/src/ic.cc#newcode1534
src/ic.cc:1534: return monomorphic_stub;
Could just be

return ComputeMonomorphicStub(...);

http://codereview.chromium.org/8356041/diff/1/src/list-inl.h
File src/list-inl.h (right):

http://codereview.chromium.org/8356041/diff/1/src/list-inl.h#newcode241
src/list-inl.h:241: List<T*>* UnwrapList(List<T*>* destination, List<Handle<T>
>* source) {
Maybe this name needs to be more obvious, like "UnwrapHandleList"....

You could avoid having to fill the destination with NULLs by doing:

ASSERT(destination.is_empty());
int length = source->length();
for (int i = 0; i < length; ++i) {
  Handle<T> handle = source->at(i);
  destination->Add(handle.is_null() ? NULL : *handle);
}
return destination;

http://codereview.chromium.org/8356041/diff/1/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/8356041/diff/1/src/objects.cc#newcode2199
src/objects.cc:2199: return (result == NULL) ? Handle<Map>() :
Handle<Map>(result);
Handle<Map>::null() a bit better than Handle<Map>().

http://codereview.chromium.org/8356041/diff/1/src/objects.cc#newcode2203
src/objects.cc:2203: Map* Map::FindTransitionedMap(MapList* candidates) {
This is still needed by the raw pointer code?  Will it be possible to handlify
it directly at some point?

http://codereview.chromium.org/8356041/diff/1/src/objects.cc#newcode5160
src/objects.cc:5160: MaybeObject* PolymorphicCodeCache::Update(MapList* maps,
Likewise, do you think it will be eventually possible to handlify the
PolymorphicCodeCache directly, or will we need to update it from raw pointer
code?

http://codereview.chromium.org/8356041/diff/1/src/stub-cache.cc
File src/stub-cache.cc (right):

http://codereview.chromium.org/8356041/diff/1/src/stub-cache.cc#newcode537
src/stub-cache.cc:537: CALL_HEAP_FUNCTION(isolate(), CompileLoadElement(*map),
Code);
Remember that these need to reset the stub compiler's failure_ field.

[fschneider, 2011-10-21 08:45:54]

DBC:

http://codereview.chromium.org/8356041/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8356041/diff/1/src/ic.cc#newcode1492
src/ic.cc:1492: if (!receiver_maps->at(current).is_null() &&
On 2011/10/20 14:35:54, ulan wrote:
> Unfortunately, receiver_maps can contain nulls (see line 1667).

If we have to check for null-handles in a lot of places, I'd consider removing
the debug-mode assert in the *-operator that checks for NULL. It does not help
much in debugging IMO. What do you guys think?

[ulan, 2011-10-21 09:52:15]

Please take another look. Implemented the suggestion about the UnwrapHandleList
and rebased.

http://codereview.chromium.org/8356041/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8356041/diff/1/src/ic.cc#newcode1492
src/ic.cc:1492: if (!receiver_maps->at(current).is_null() &&
On 2011/10/21 08:45:55, fschneider wrote:
> On 2011/10/20 14:35:54, ulan wrote:
> > Unfortunately, receiver_maps can contain nulls (see line 1667).
> 
> If we have to check for null-handles in a lot of places, I'd consider removing
> the debug-mode assert in the *-operator that checks for NULL. It does not help
> much in debugging IMO. What do you guys think?
I would also prefer to remove the assert. But I am not sure if there are people
relying on it.

http://codereview.chromium.org/8356041/diff/1/src/ic.cc#newcode1534
src/ic.cc:1534: return monomorphic_stub;
On 2011/10/21 06:48:13, Kevin Millikin wrote:
> Could just be
> 
> return ComputeMonomorphicStub(...);

Done.

http://codereview.chromium.org/8356041/diff/1/src/list-inl.h
File src/list-inl.h (right):

http://codereview.chromium.org/8356041/diff/1/src/list-inl.h#newcode241
src/list-inl.h:241: List<T*>* UnwrapList(List<T*>* destination, List<Handle<T>
>* source) {
On 2011/10/21 06:48:13, Kevin Millikin wrote:
> Maybe this name needs to be more obvious, like "UnwrapHandleList"....
> 
> You could avoid having to fill the destination with NULLs by doing:
> 
> ASSERT(destination.is_empty());
> int length = source->length();
> for (int i = 0; i < length; ++i) {
>   Handle<T> handle = source->at(i);
>   destination->Add(handle.is_null() ? NULL : *handle);
> }
> return destination;

Done.

http://codereview.chromium.org/8356041/diff/1/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/8356041/diff/1/src/objects.cc#newcode2199
src/objects.cc:2199: return (result == NULL) ? Handle<Map>() :
Handle<Map>(result);
On 2011/10/21 06:48:13, Kevin Millikin wrote:
> Handle<Map>::null() a bit better than Handle<Map>().

Done.

http://codereview.chromium.org/8356041/diff/1/src/objects.cc#newcode2203
src/objects.cc:2203: Map* Map::FindTransitionedMap(MapList* candidates) {
On 2011/10/21 06:48:13, Kevin Millikin wrote:
> This is still needed by the raw pointer code?  Will it be possible to handlify
> it directly at some point?

It is used in hydrogen code (HandlePolymorphicElementAccess), but it shouldn't
be difficult to replace raw pointer lists with handle lists. I will do it in
separate CL.

http://codereview.chromium.org/8356041/diff/1/src/objects.cc#newcode5160
src/objects.cc:5160: MaybeObject* PolymorphicCodeCache::Update(MapList* maps,
On 2011/10/21 06:48:13, Kevin Millikin wrote:
> Likewise, do you think it will be eventually possible to handlify the
> PolymorphicCodeCache directly, or will we need to update it from raw pointer
> code?
ComputeStub seems to be the only user of the PolymorphicCodeCache, so it should
be possible to handlify.

http://codereview.chromium.org/8356041/diff/1/src/stub-cache.cc
File src/stub-cache.cc (right):

http://codereview.chromium.org/8356041/diff/1/src/stub-cache.cc#newcode537
src/stub-cache.cc:537: CALL_HEAP_FUNCTION(isolate(), CompileLoadElement(*map),
Code);
On 2011/10/21 06:48:13, Kevin Millikin wrote:
> Remember that these need to reset the stub compiler's failure_ field.

Done.

[Kevin Millikin (Chromium), 2011-10-21 10:13:48]

LGTM.

http://codereview.chromium.org/8356041/diff/6004/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/8356041/diff/6004/src/objects.cc#newcode5152
src/objects.cc:5152: (raw_maps.Clear(),
That's a bit annoying, but unavoidable.  I suppose you could give
UnwrapHandleList the semantics of clearing destination, but that seems wrong.