Cache maps for slow case objects.

Cache maps for slow case objects.



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

[Anton Muhin, 2010-07-27 11:49:09]

That looks mostly LGTM.

My main complaint is absence of tests.  I'd suggest add both tests for various
transitions and for GC in presence of several threads (like in
test/cctest/test-threads/JSFunctionResultCachesInTwoThreads).

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

http://codereview.chromium.org/3032028/diff/1/4#newcode780
src/heap.cc:780: ClearNormalizedMapCaches();
maybe you should only clear those when compacting?

http://codereview.chromium.org/3032028/diff/1/6
File src/objects-debug.cc (right):

http://codereview.chromium.org/3032028/diff/1/6#newcode1366
src/objects-debug.cc:1366: for (int i = 0; i < length(); i++) {
if cache size is big, you might want to put it under FLAG_enable_slow_asserts

http://codereview.chromium.org/3032028/diff/1/6#newcode1368
src/objects-debug.cc:1368: ASSERT(e->IsMap() || e->IsUndefined());
e->Verify() as well?

http://codereview.chromium.org/3032028/diff/1/6#newcode1368
src/objects-debug.cc:1368: ASSERT(e->IsMap() || e->IsUndefined());
do you want to add a check that all the maps are slow?

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

http://codereview.chromium.org/3032028/diff/1/8#newcode2118
src/objects.cc:2118: int hash = (static_cast<uint32_t>(
please, add comments why you don't need dependencies on other fields of map.

http://codereview.chromium.org/3032028/diff/1/8#newcode2119
src/objects.cc:2119: reinterpret_cast<uintptr_t>(fast->constructor())) >> 2);
why not use two additional bits on x64?  if you would first >> 2 and then
static_cast<uint32_t>, you would get it for free, now?

And why use uint32_t, maybe just unsigned?

http://codereview.chromium.org/3032028/diff/1/8#newcode2163
src/objects.cc:2163: set(i, Heap::undefined_value());
you might prefer to use set_undefined(i) here---it even should be slightly
faster (as it doesn't update RS)

http://codereview.chromium.org/3032028/diff/1/8#newcode2240
src/objects.cc:2240: if (instance_size_delta != 0) {
should we assert that instance_size_delta >= 0?

http://codereview.chromium.org/3032028/diff/1/8#newcode2241
src/objects.cc:2241: Heap::CreateFillerObjectAt(this->address() +
new_instance_size,
maybe add some asserts like scavenger() == Hep::GetScavenger, etc.

http://codereview.chromium.org/3032028/diff/1/8#newcode2255
src/objects.cc:2255: int new_instance_size = map()->instance_size() -
instance_size_delta;
why it is different than code above?

http://codereview.chromium.org/3032028/diff/1/8#newcode2269
src/objects.cc:2269:
map()->set_instance_descriptors(Heap::empty_descriptor_array());
maybe add to Verify assertion that instance_descriptors() are always empty for
cached slow maps.

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

http://codereview.chromium.org/3032028/diff/1/9#newcode2398
src/objects.h:2398: static int Hash(Map* fast);
could/should we simplify the interface?  For example if double calculation of
hash is not too expensive, maybe we should just have two methods: Get(Map* fast)
and Set(Map* fast, Map* slow)?  If it's expensive, maybe something like Get(Map*
fast, bool* was_created) which would pack two operations into one?

IsMatchFor looks like really low level detail for me.

[Vladislav Kaznacheev, 2010-07-28 12:01:15]

Refactored. Please review again.

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

http://codereview.chromium.org/3032028/diff/1/4#newcode780
src/heap.cc:780: ClearNormalizedMapCaches();
On 2010/07/27 11:49:09, antonm1 wrote:
> maybe you should only clear those when compacting?

I clear the cache here so that unused slow maps are disposed.

http://codereview.chromium.org/3032028/diff/1/6
File src/objects-debug.cc (right):

http://codereview.chromium.org/3032028/diff/1/6#newcode1366
src/objects-debug.cc:1366: for (int i = 0; i < length(); i++) {
On 2010/07/27 11:49:09, antonm1 wrote:
> if cache size is big, you might want to put it under FLAG_enable_slow_asserts

Done.

http://codereview.chromium.org/3032028/diff/1/6#newcode1368
src/objects-debug.cc:1368: ASSERT(e->IsMap() || e->IsUndefined());
On 2010/07/27 11:49:09, antonm1 wrote:
> do you want to add a check that all the maps are slow?

Done.

http://codereview.chromium.org/3032028/diff/1/6#newcode1368
src/objects-debug.cc:1368: ASSERT(e->IsMap() || e->IsUndefined());
On 2010/07/27 11:49:09, antonm1 wrote:
> e->Verify() as well?

Done.

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

http://codereview.chromium.org/3032028/diff/1/8#newcode2118
src/objects.cc:2118: int hash = (static_cast<uint32_t>(
On 2010/07/27 11:49:09, antonm1 wrote:
> please, add comments why you don't need dependencies on other fields of map.

Done.

http://codereview.chromium.org/3032028/diff/1/8#newcode2119
src/objects.cc:2119: reinterpret_cast<uintptr_t>(fast->constructor())) >> 2);
On 2010/07/27 11:49:09, antonm1 wrote:
> why not use two additional bits on x64?  if you would first >> 2 and then
> static_cast<uint32_t>, you would get it for free, now?
> 
> And why use uint32_t, maybe just unsigned? 

This is how normally done in other places inn V8 where we hash pointers. 2 extra
bit are not likely to change much.

http://codereview.chromium.org/3032028/diff/1/8#newcode2163
src/objects.cc:2163: set(i, Heap::undefined_value());
On 2010/07/27 11:49:09, antonm1 wrote:
> you might prefer to use set_undefined(i) here---it even should be slightly
> faster (as it doesn't update RS)

Done.

http://codereview.chromium.org/3032028/diff/1/8#newcode2240
src/objects.cc:2240: if (instance_size_delta != 0) {
On 2010/07/27 11:49:09, antonm1 wrote:
> should we assert that instance_size_delta >= 0?

Done.

http://codereview.chromium.org/3032028/diff/1/8#newcode2241
src/objects.cc:2241: Heap::CreateFillerObjectAt(this->address() +
new_instance_size,
On 2010/07/27 11:49:09, antonm1 wrote:
> maybe add some asserts like scavenger() == Hep::GetScavenger, etc.

Done in NormalizedMapVerify

http://codereview.chromium.org/3032028/diff/1/8#newcode2255
src/objects.cc:2255: int new_instance_size = map()->instance_size() -
instance_size_delta;
On 2010/07/27 11:49:09, antonm1 wrote:
> why it is different than code above?

refactored

http://codereview.chromium.org/3032028/diff/1/8#newcode2269
src/objects.cc:2269:
map()->set_instance_descriptors(Heap::empty_descriptor_array());
On 2010/07/27 11:49:09, antonm1 wrote:
> maybe add to Verify assertion that instance_descriptors() are always empty for
> cached slow maps.

Done.

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

http://codereview.chromium.org/3032028/diff/1/9#newcode2398
src/objects.h:2398: static int Hash(Map* fast);
On 2010/07/27 11:49:09, antonm1 wrote:
> could/should we simplify the interface?  For example if double calculation of
> hash is not too expensive, maybe we should just have two methods: Get(Map*
fast)
> and Set(Map* fast, Map* slow)?  If it's expensive, maybe something like
Get(Map*
> fast, bool* was_created) which would pack two operations into one?
> 
> IsMatchFor looks like really low level detail for me.
Simplified quite a lot.

[antonm, 2010-07-28 12:14:51]

LGTM.  Last version looks notably better imho.

http://codereview.chromium.org/3032028/diff/12001/13007
File src/objects.cc (right):

http://codereview.chromium.org/3032028/diff/12001/13007#newcode2142
src/objects.cc:2142: Counters::normalized_maps.Increment();
shouldn't there be another counter which ticks when NormalizedMapCache::Get is
invoked?

[Vladislav Kaznacheev, 2010-07-28 12:21:39]

http://codereview.chromium.org/3032028/diff/12001/13007
File src/objects.cc (right):

http://codereview.chromium.org/3032028/diff/12001/13007#newcode2142
src/objects.cc:2142: Counters::normalized_maps.Increment();
On 2010/07/28 12:14:51, antonm wrote:
> shouldn't there be another counter which ticks when NormalizedMapCache::Get is
> invoked?

We already have it: Counters::props_to_dictionary which is incremented in
NormalizedProperties every time it is called.