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. http://codereview.chromium.org/3032028/show -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev
