[Isolates] Add a pointer to Heap to a meta map.

[Isolates] Add a pointer to Heap to a meta map.

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

[Dmitry Titov, 2010-08-04 01:26:09]

Also modified calls in objects-inl.h to use this new method to retrieve a heap.
This speeds up some tests I'm running by 5-8%.

[Vitaly Repeshko, 2010-08-04 18:15:43]

1. You changed the layout of all maps (not just the meta-map), so you could put
the isolate pointer in every map and save one indirection. But I don't think
this memory usage increase is acceptable. This is why I originally suggested
using meta-map. Its layout can be special.

2. I don't think we can hope to be able to access the map during GC. Pretty
arbitrary stuff can be encoded in the first word of an object. (Not just the map
pointer with extra bits, see MapWord.) On the other hand, the set of functions
on objects that the GC touches should be pretty limited: we could duplicate them
or add extra parameters, etc. So most functions should still be able to use
map()->map()->heap() (guarded with asserts and wrapped in a helper function) and
only this special set of functions will require some workarounds.

[Dmitry Titov, 2010-08-05 02:30:07]

Updated the patch to use the Scavenger field for Heap in the meta map. This
field is never used in meta map, and can be shared.

I considered making a different layout for meta map but there are a lot of
places in code that assume all maps to be the same size and it'd be too much
change... Sharing with Scavenger is even convenient because scavenger is also a
real pointer (not to heap object). I had to 'restore' heap pointer in meta map
when snapshot is read back, but it's simple.

I am still trying to figure out how many places we have that try to access the
heap via this mechanism during GC. I have a feeling there can be quite a few,
but need some time to figure out.

The final patch still comes (with GC issue figured out).

[Dmitry Titov, 2010-08-05 02:56:09]

the stuff we call during GC is numerous... Look at Map::CreateBackPointers() for
example. Maybe I can have a bit in the map saying "the map pointer was changed
to somethign else, use Isoalte::Current()"? 
Still thinkign about this, input is welcome.

[zarko, 2010-08-05 21:04:23]

http://codereview.chromium.org/3089005/diff/4001/5003
File src/isolate.h (right):

http://codereview.chromium.org/3089005/diff/4001/5003#newcode1028
src/isolate.h:1028: #define GET_ISOLATE(heap_pointer) ((Isolate*)(  \
Would it be better to call this something like HEAP_ISOLATE()?

[Dmitry Titov, 2010-08-05 21:19:38]

On 2010/08/05 21:04:23, zarko wrote:
> http://codereview.chromium.org/3089005/diff/4001/5003
> File src/isolate.h (right):
> 
> http://codereview.chromium.org/3089005/diff/4001/5003#newcode1028
> src/isolate.h:1028: #define GET_ISOLATE(heap_pointer) ((Isolate*)(  \
> Would it be better to call this something like HEAP_ISOLATE()?

I would understand HEAP_FROM_ISOLATE, GET_HEAP_FROM_ISOLATE or
COMPUTE_HEAP_POINTER, but why HEAP_ISOLATE? 2 noons?

[Dmitry Titov, 2010-08-05 21:21:24]

/noons/nouns/

[Dmitry Titov, 2010-08-05 23:39:20]

I think I've addressed Vitaly's comments, it is ready for another look.

1. The heap pointer is now sharing the field with scavenge callback pointer in
meta map (since meta map always has it NULL). So there is no special meta map
layout and no increase in map size. The special has_heap_in_scavenger_field()
check is used for ASSERTS.

2. Run tests and figured out all cases where GC codepath was trying to get heap
out of map. Indeed, there was not many places like that (thanks to oddball patch
too!) so I've added just enough heap pointer passing to make the GC avoid the
HeapObject::GetHeap() call. Assert in GetHeap() prevents incorrect usage.

[Dmitry Titov, 2010-08-06 20:07:09]

Also: from chat with Luke, we think GET_ISOLATE should be ISOLATE_FROM_HEAP,
I'll rename (or merge with Luke's patch if it lands earlier).

[Vitaly Repeshko, 2010-08-06 20:40:46]

LGTM! (with comments:)


Thanks,
Vitaly

http://codereview.chromium.org/3089005/diff/7002/16001
File src/heap.cc (right):

http://codereview.chromium.org/3089005/diff/7002/16001#newcode1363
src/heap.cc:1363: reinterpret_cast<Map*>(result)->set_bit_field2(0);
It'd be nice to restore the original initialization order. See comments in
objects.h.

http://codereview.chromium.org/3089005/diff/7002/16001#newcode1381
src/heap.cc:1381: map->set_bit_field2((1 << Map::kIsExtensible) | (1 <<
Map::kHasFastElements));
Same here.

http://codereview.chromium.org/3089005/diff/7002/16002
File src/isolate.h (right):

http://codereview.chromium.org/3089005/diff/7002/16002#newcode1028
src/isolate.h:1028: #define GET_ISOLATE(heap_pointer) ((Isolate*)(  \
Nice hack :) A few suggestions:
1. Why is this not Heap::isolate()?
2. You can use OFFSET_OF (from globals.h) in its implementation (maybe modulo a
friend declaration).
3. In general, please use C++-style casts instead of C-style ones. The latter
don't lint.

http://codereview.chromium.org/3089005/diff/7002/16004
File src/objects-inl.h (right):

http://codereview.chromium.org/3089005/diff/7002/16004#newcode1469
src/objects-inl.h:1469: void FixedArray::set_undefined(Heap* heap, int index) {
To avoid code duplication the old versions of these new functions should
delegate to them. Is there a reason this is not done in this change?

http://codereview.chromium.org/3089005/diff/7002/16004#newcode1480
src/objects-inl.h:1480: WRITE_FIELD(this, kHeaderSize + index * kPointerSize,
GetHeap()->null_value());
Long line.

http://codereview.chromium.org/3089005/diff/7002/16004#newcode1494
src/objects-inl.h:1494: WRITE_FIELD(this, kHeaderSize + index * kPointerSize,
GetHeap()->the_hole_value());
Long line.

http://codereview.chromium.org/3089005/diff/7002/16004#newcode3013
src/objects-inl.h:3013: if (array->map() == GetHeap()->fixed_array_map()) {
Can be rewritten like this:
if (map()->has_fast_elements()) {
  ASSERT(array->map() == GetHeap()->fixed_array_map());
  return FAST_ELEMENTS;
}
ASSERT(array->IsDictionary());
return DICTIONARY_ELEMENTS;

http://codereview.chromium.org/3089005/diff/7002/16006
File src/objects.h (right):

http://codereview.chromium.org/3089005/diff/7002/16006#newcode1003
src/objects.h:1003: // The Heap the object was allocated in. Used also to access
Isolate.
Please document that it mustn't be used during GC.

http://codereview.chromium.org/3089005/diff/7002/16006#newcode1745
src/objects.h:1745: inline bool IsEmpty(Heap* heap);
Most likely this is unnecessary. The invariant is that if this->length() <= 2
then this == empty_descriptor_array. We can guard this with asserts and
implement IsEmpty and number_of_descriptors in terms of this->length().

http://codereview.chromium.org/3089005/diff/7002/16006#newcode3029
src/objects.h:3029: inline Heap* heap();
Move this next to the scavenger accessors.

http://codereview.chromium.org/3089005/diff/7002/16006#newcode3114
src/objects.h:3114: // Meta map has a Heap pointer instead of Scavanger.
Can we rely solely on the instance_type check? The meta-map should be the only
one with instance_type == MAP. (We can also check this == map() when not in GC.)
The instance_type field is set before other fields so we should be able to
restore the initialization order in heap.cc and avoid having an extra bit.

http://codereview.chromium.org/3089005/diff/7002/16008
File src/stub-cache.cc (right):

http://codereview.chromium.org/3089005/diff/7002/16008#newcode704
src/stub-cache.cc:704: ASSERT(!result->IsHeapObject() || result !=
HEAP->undefined_value());
After Luke's changes IsUndefined should work here.

[Dmitry Titov, 2010-08-06 22:48:41]

Great comments, thanks!
Fixed all, except:

> 2. You can use OFFSET_OF (from globals.h) in its implementation (maybe modulo
a
> friend declaration).

This indeed would require making Isoalte::heap_ public or making these big
classes friends, neither is especially nice. Made it Heap::isolate() and used
C++ casts though.