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. http://codereview.chromium.org/3089005/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
