Per-Isolate cache for polymorphic stubs

Per-Isolate cache for polymorphic stubs

BUG=1385
TEST=Existing tests still pass; running d8 with --dump-counters shows fewer polymorphic stubs being compiled

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

[Jakob Kummerow, 2011-05-31 14:11:39]

PTAL.

[Mads Ager (chromium), 2011-06-01 08:45:53]

The usages of C++ allocated MapLists look fishy. The rest is looking good.

http://codereview.chromium.org/7094003/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7094003/diff/1/src/heap.cc#newcode1646
src/heap.cc:1646: code_cache->set_cache(undefined_value());
You actually don't need this. Structs have all fields initialized with
undefined. You could just have a one-liner:

return AllocateStruct(POLYMORPHIC_CODE_CACHE_TYPE);

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

http://codereview.chromium.org/7094003/diff/1/src/ic.cc#newcode1632
src/ic.cc:1632: // If the maximum number of receiver maps has been exceeded, use
the Generic
Generic -> generic?

http://codereview.chromium.org/7094003/diff/1/src/ic.cc#newcode1650
src/ic.cc:1650: CodeList handler_ics(KeyedIC::kMaxKeyedPolymorphism);
Not your code. :-) Don't we always have target_receiver_maps.length() handler
ics? If so, we don't need to over allocate for the list.

http://codereview.chromium.org/7094003/diff/1/src/ic.cc#newcode1665
src/ic.cc:1665: MaybeObject* maybe_update = cache->Update(&target_receiver_maps,
flags, stub);
Not sure I understand the ownership for |target_receiver_maps|. You are passing
a pointer to a local and using that as part of the key in a hash table? Is there
copying going on somewhere? If not, this looks unsafe.

http://codereview.chromium.org/7094003/diff/1/src/objects.cc
File src/objects.cc (left):

http://codereview.chromium.org/7094003/diff/1/src/objects.cc#oldcode2564
src/objects.cc:2564: (slow->bit_field3() & ~(1<<Map::kIsShared)) ==
fast->bit_field3();
Here we only ignore the shared bit on |slow|. Is that a bug? You have updated it
to ignore on both which seems like the right thing, but I haven't double
checked.

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

http://codereview.chromium.org/7094003/diff/1/src/objects.cc#newcode4133
src/objects.cc:4133: : maps_(maps),
This looks unsafe. I think you have to copy the maps to a fixed array? Maybe we
should do that in the IC code?

http://codereview.chromium.org/7094003/diff/1/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7094003/diff/1/src/objects.h#newcode3903
src/objects.h:3903: // Compares this map to another. |mode| affects how |other|
is looked at.
Could you update the comment to state which effect |mode| has? It seems to me
that Equals is actually the wrong name here. This does not check equality. It
checks that performing some operation to this map would produce other. I would
like a better name for that.

[Jakob Kummerow, 2011-06-01 10:15:27]

Thanks for the comments. Responses below.

http://codereview.chromium.org/7094003/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7094003/diff/1/src/heap.cc#newcode1646
src/heap.cc:1646: code_cache->set_cache(undefined_value());
On 2011/06/01 08:45:53, Mads Ager wrote:
> You actually don't need this. Structs have all fields initialized with
> undefined. You could just have a one-liner:
> 
> return AllocateStruct(POLYMORPHIC_CODE_CACHE_TYPE);

Nice! Done.

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

http://codereview.chromium.org/7094003/diff/1/src/ic.cc#newcode1632
src/ic.cc:1632: // If the maximum number of receiver maps has been exceeded, use
the Generic
On 2011/06/01 08:45:53, Mads Ager wrote:
> Generic -> generic?

Done.

http://codereview.chromium.org/7094003/diff/1/src/ic.cc#newcode1650
src/ic.cc:1650: CodeList handler_ics(KeyedIC::kMaxKeyedPolymorphism);
On 2011/06/01 08:45:53, Mads Ager wrote:
> Not your code. :-) Don't we always have target_receiver_maps.length() handler
> ics? If so, we don't need to over allocate for the list.

Done.

http://codereview.chromium.org/7094003/diff/1/src/ic.cc#newcode1665
src/ic.cc:1665: MaybeObject* maybe_update = cache->Update(&target_receiver_maps,
flags, stub);
On 2011/06/01 08:45:53, Mads Ager wrote:
> Not sure I understand the ownership for |target_receiver_maps|. You are
passing
> a pointer to a local and using that as part of the key in a hash table? Is
there
> copying going on somewhere? If not, this looks unsafe.

Ownership is local. cache->Update takes a weak reference, and only makes use of
it before the call returns (--> while the local variable is still alive).
(See also my response to your comment in objects.cc on the same issue.)

http://codereview.chromium.org/7094003/diff/1/src/objects.cc
File src/objects.cc (left):

http://codereview.chromium.org/7094003/diff/1/src/objects.cc#oldcode2564
src/objects.cc:2564: (slow->bit_field3() & ~(1<<Map::kIsShared)) ==
fast->bit_field3();
On 2011/06/01 08:45:53, Mads Ager wrote:
> Here we only ignore the shared bit on |slow|. Is that a bug? You have updated
it
> to ignore on both which seems like the right thing, but I haven't double
> checked.

If I understand the code correctly, for all cases where this was used before,
|fast| never had the "shared" flag set, so this comparison was OK. Since I
introduced an additional use case for this code where this assumption was not
true, I had to make it more flexible by explicitly ignoring |fast|'s "shared"
flag.

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

http://codereview.chromium.org/7094003/diff/1/src/objects.cc#newcode4133
src/objects.cc:4133: : maps_(maps),
On 2011/06/01 08:45:53, Mads Ager wrote:
> This looks unsafe. I think you have to copy the maps to a fixed array? Maybe
we
> should do that in the IC code?

Note how the |key| objects constructed from this class in
PolymorphicCodeCacheHashTable are short-lived local variables. Only the return
value of AsObject is persisted in the hash table, and inside that method I
actually do copy the maps to a fixed array. 
I'm pretty sure that the code is safe in its current form, but agree that this
ownership issue is somewhat convoluted and therefore fragile wrt. future
modifications, so I'm happy to either put in comments explaining the situation
and caveats, or refactor (e.g. performing the copying in ic.cc as you suggested
and passing the FixedArray around). Take your pick :-)

http://codereview.chromium.org/7094003/diff/1/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7094003/diff/1/src/objects.h#newcode3903
src/objects.h:3903: // Compares this map to another. |mode| affects how |other|
is looked at.
On 2011/06/01 08:45:53, Mads Ager wrote:
> Could you update the comment to state which effect |mode| has? It seems to me
> that Equals is actually the wrong name here. This does not check equality. It
> checks that performing some operation to this map would produce other. I would
> like a better name for that.

OK. I agree that "Equals" isn't a good name. Here are some suggestions:
Comment: Compares this map to another to see if they describe equivalent
objects. |mode| specifies whether |other|'s inobject_properties() should be
ignored or not.
Method name: Matches()? IsEquivalent()? IsHashTableHit()?
Which one do you like best?

[Mads Ager (chromium), 2011-06-01 10:36:54]

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

http://codereview.chromium.org/7094003/diff/1/src/ic.cc#newcode1665
src/ic.cc:1665: MaybeObject* maybe_update = cache->Update(&target_receiver_maps,
flags, stub);
On 2011/06/01 10:15:27, jkummerow wrote:
> On 2011/06/01 08:45:53, Mads Ager wrote:
> > Not sure I understand the ownership for |target_receiver_maps|. You are
> passing
> > a pointer to a local and using that as part of the key in a hash table? Is
> there
> > copying going on somewhere? If not, this looks unsafe.
> 
> Ownership is local. cache->Update takes a weak reference, and only makes use
of
> it before the call returns (--> while the local variable is still alive).
> (See also my response to your comment in objects.cc on the same issue.)

Thanks for explaining. I forgot how the HashTableKey stuff works. :-)

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

http://codereview.chromium.org/7094003/diff/1/src/objects.cc#newcode4133
src/objects.cc:4133: : maps_(maps),
On 2011/06/01 10:15:27, jkummerow wrote:
> On 2011/06/01 08:45:53, Mads Ager wrote:
> > This looks unsafe. I think you have to copy the maps to a fixed array? Maybe
> we
> > should do that in the IC code?
> 
> Note how the |key| objects constructed from this class in
> PolymorphicCodeCacheHashTable are short-lived local variables. Only the return
> value of AsObject is persisted in the hash table, and inside that method I
> actually do copy the maps to a fixed array. 
> I'm pretty sure that the code is safe in its current form, but agree that this
> ownership issue is somewhat convoluted and therefore fragile wrt. future
> modifications, so I'm happy to either put in comments explaining the situation
> and caveats, or refactor (e.g. performing the copying in ic.cc as you
suggested
> and passing the FixedArray around). Take your pick :-)

Ah, yeah. Sorry I forgot how this works. You are absolutely right. I think we
should keep what you have here so we avoid the fixed array allocation on
lookups. If you just add a comment somewhere around here that the key objects
are not stored in the hash table, are temporary, and that it is therefore safe
to keep a pointer to the MapList I'll be happy. :-)

http://codereview.chromium.org/7094003/diff/1/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7094003/diff/1/src/objects.h#newcode3903
src/objects.h:3903: // Compares this map to another. |mode| affects how |other|
is looked at.
On 2011/06/01 10:15:27, jkummerow wrote:
> On 2011/06/01 08:45:53, Mads Ager wrote:
> > Could you update the comment to state which effect |mode| has? It seems to
me
> > that Equals is actually the wrong name here. This does not check equality.
It
> > checks that performing some operation to this map would produce other. I
would
> > like a better name for that.
> 
> OK. I agree that "Equals" isn't a good name. Here are some suggestions:
> Comment: Compares this map to another to see if they describe equivalent
> objects. |mode| specifies whether |other|'s inobject_properties() should be
> ignored or not.

They are not really ignored. If you are about to clear inobject properties you
want to find a map that is the same as this one except that it has exactly zero
inobject properties.

Should we have two methods instead? One that is called EquivalentTo which only
ignores the shared bit (and takes no mode) and one called
EquivalentToForNormalization which takes into account whether or not inobject
properties remain or not (takes a mode). They can share the implementation but
it might make the code more readable?

[Jakob Kummerow, 2011-06-01 12:03:09]

Here's an update with the changes we've discussed so far.

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

http://codereview.chromium.org/7094003/diff/1/src/objects.cc#newcode4133
src/objects.cc:4133: : maps_(maps),
On 2011/06/01 10:36:54, Mads Ager wrote:
> On 2011/06/01 10:15:27, jkummerow wrote:
> > On 2011/06/01 08:45:53, Mads Ager wrote:
> > > This looks unsafe. I think you have to copy the maps to a fixed array?
Maybe
> > we
> > > should do that in the IC code?
> > 
> > Note how the |key| objects constructed from this class in
> > PolymorphicCodeCacheHashTable are short-lived local variables. Only the
return
> > value of AsObject is persisted in the hash table, and inside that method I
> > actually do copy the maps to a fixed array. 
> > I'm pretty sure that the code is safe in its current form, but agree that
this
> > ownership issue is somewhat convoluted and therefore fragile wrt. future
> > modifications, so I'm happy to either put in comments explaining the
situation
> > and caveats, or refactor (e.g. performing the copying in ic.cc as you
> suggested
> > and passing the FixedArray around). Take your pick :-)
> 
> Ah, yeah. Sorry I forgot how this works. You are absolutely right. I think we
> should keep what you have here so we avoid the fixed array allocation on
> lookups. If you just add a comment somewhere around here that the key objects
> are not stored in the hash table, are temporary, and that it is therefore safe
> to keep a pointer to the MapList I'll be happy. :-)

Done.

http://codereview.chromium.org/7094003/diff/1/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7094003/diff/1/src/objects.h#newcode3903
src/objects.h:3903: // Compares this map to another. |mode| affects how |other|
is looked at.
On 2011/06/01 10:36:54, Mads Ager wrote:
> On 2011/06/01 10:15:27, jkummerow wrote:
> > On 2011/06/01 08:45:53, Mads Ager wrote:
> > > Could you update the comment to state which effect |mode| has? It seems to
> me
> > > that Equals is actually the wrong name here. This does not check equality.
> It
> > > checks that performing some operation to this map would produce other. I
> would
> > > like a better name for that.
> > 
> > OK. I agree that "Equals" isn't a good name. Here are some suggestions:
> > Comment: Compares this map to another to see if they describe equivalent
> > objects. |mode| specifies whether |other|'s inobject_properties() should be
> > ignored or not.
> 
> They are not really ignored. If you are about to clear inobject properties you
> want to find a map that is the same as this one except that it has exactly
zero
> inobject properties.
> 
> Should we have two methods instead? One that is called EquivalentTo which only
> ignores the shared bit (and takes no mode) and one called
> EquivalentToForNormalization which takes into account whether or not inobject
> properties remain or not (takes a mode). They can share the implementation but
> it might make the code more readable?

Done.

[Mads Ager (chromium), 2011-06-01 12:09:00]

LGTM!

[danno, 2011-06-03 13:39:56]

LGTM