First round of comments. Only minor things. This is looking good! :-)
http://codereview.chromium.org/7089002/diff/5002/src/heap.cc File src/heap.cc (right): http://codereview.chromium.org/7089002/diff/5002/src/heap.cc#newcode3854 src/heap.cc:3854: reinterpret_cast<FixedDoubleArray*>(obj)->set_map(fixed_double_array_map()); No initialization of the body at all? Could you call this AllocateUninitializedFixedDoubleArray? The comment for this method should state that the contents of the array is uninitialized. http://codereview.chromium.org/7089002/diff/5002/src/heap.h File src/heap.h (right): http://codereview.chromium.org/7089002/diff/5002/src/heap.h#newcode625 src/heap.h:625: // Allocates a fixed double array initialized with undefined values Period at end of comment. Let's call this AllocateUninitializedFixedDoubleArray? The comment indicated to me that it initializes the body with 'undefined'. http://codereview.chromium.org/7089002/diff/5002/src/objects-inl.h File src/objects-inl.h (right): http://codereview.chromium.org/7089002/diff/5002/src/objects-inl.h#newcode1607 src/objects-inl.h:1607: int FixedDoubleArray::length() { For length you can use use the INT_ACCESSORS macro? http://codereview.chromium.org/7089002/diff/5002/src/objects-inl.h#newcode1649 src/objects-inl.h:1649: memcpy(FIELD_ADDR(this, kHeaderSize), Use our own MemCopy? http://codereview.chromium.org/7089002/diff/5002/src/objects-inl.h#newcode1666 src/objects-inl.h:1666: if (hole_or_object == heap->the_hole_value()) { hole_or_object->IsTheHole() http://codereview.chromium.org/7089002/diff/5002/src/objects.cc File src/objects.cc (right): http://codereview.chromium.org/7089002/diff/5002/src/objects.cc#newcode62 src/objects.cc:62: uint64_t FixedDoubleArray:: kCanonoicalNonHoleNanLower32 = 0x7FF00000; remove space after '::' Can't you use the V8_UINT64_C macro to avoid the lower32 and shifts here? http://codereview.chromium.org/7089002/diff/5002/src/objects.cc#newcode6951 src/objects.cc:6951: elems->set(i, obj, UPDATE_WRITE_BARRIER); Why not use the WriteBarrierMode of the new elements backing store here? If the new elements are in new space you do not need to update the write barrier. You do not allow the allocations here to cause GC, so the elments cannot be moved out of new space while you are in this method. http://codereview.chromium.org/7089002/diff/5002/src/objects.cc#newcode7802 src/objects.cc:7802: if (!value_is_smi && if (!value->IsNumber()) which covers IsSmi() and IsHeapNumber(). http://codereview.chromium.org/7089002/diff/5002/src/objects.cc#newcode7807 src/objects.cc:7807: SetFastElementsCapacityAndLength(elms_length, elms_length); Should you really pass in elms_length for both here? What if the object is a JSArray and the fixed array backing store is larger than the length? It seems that this call would update the array length to be larger than it should be? http://codereview.chromium.org/7089002/diff/5002/src/objects.cc#newcode7817 src/objects.cc:7817: // Check whether there is extra space in fixed array.. .. -> . in the fixed array? http://codereview.chromium.org/7089002/diff/5002/src/objects.h File src/objects.h (right): http://codereview.chromium.org/7089002/diff/5002/src/objects.h#newcode2117 src/objects.h:2117: // FixedDoubleArray describes fixed-sized arrays with element type double Period at end of comment. http://codereview.chromium.org/7089002/diff/5002/src/objects.h#newcode2141 src/objects.h:2141: // The folllwing can't be declared inline as const static following http://codereview.chromium.org/7089002/diff/5002/src/objects.h#newcode2143 src/objects.h:2143: static uint64_t kCanonoicalNonHoleNanLower32; Canonical. http://codereview.chromium.org/7089002/diff/5002/src/objects.h#newcode2144 src/objects.h:2144: static uint64_t kCanonoicalNonHoleNanInt64; Canonical. http://codereview.chromium.org/7089002/diff/5002/src/objects.h#newcode2163 src/objects.h:2163: // Length is smi tagged when it is stored. Is it? Didn't we just use INT_ACCESSOR? You could use kIntSize for the length and use the OBJECT_POINTER_ALIGN macro to get the pointer-size aligned header size on all platforms. That will make it clear that there are 32-bits unused in the header of 64-bit. http://codereview.chromium.org/7089002/diff/5002/src/objects.h#newcode3991 src/objects.h:3991: // descriptors and the fast elements bit set. These comments should be updated? The fast elements bit is no longer a bit but a complete elements type, right? http://codereview.chromium.org/7089002/diff/5002/test/mjsunit/unbox-double-arrays.js File test/mjsunit/unbox-double-arrays.js (right): http://codereview.chromium.org/7089002/diff/5002/test/mjsunit/unbox-double-arrays.js#newcode32 test/mjsunit/unbox-double-arrays.js:32: a[i] = i+.5; i + 0.5; ? http://codereview.chromium.org/7089002/diff/5002/test/mjsunit/unbox-double-arrays.js#newcode39 test/mjsunit/unbox-double-arrays.js:39: assertEquals(i+.5, foo[i]); Ditto. http://codereview.chromium.org/7089002/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
