please take another look
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()); On 2011/06/06 07:58:23, Mads Ager wrote:
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.
Done. 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 On 2011/06/06 07:58:23, Mads Ager wrote:
Period at end of comment.
Let's call this AllocateUninitializedFixedDoubleArray? The comment
indicated to
me that it initializes the body with 'undefined'.
Done. 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() { On 2011/06/06 07:58:23, Mads Ager wrote:
For length you can use use the INT_ACCESSORS macro?
Done. http://codereview.chromium.org/7089002/diff/5002/src/objects-inl.h#newcode1649 src/objects-inl.h:1649: memcpy(FIELD_ADDR(this, kHeaderSize), On 2011/06/06 07:58:23, Mads Ager wrote:
Use our own MemCopy?
Done. 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()) { On 2011/06/06 07:58:23, Mads Ager wrote:
hole_or_object->IsTheHole()
Done. 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; On 2011/06/06 07:58:23, Mads Ager wrote:
remove space after '::'
Can't you use the V8_UINT64_C macro to avoid the lower32 and shifts
here? This macro only works with x64 builds. It reports that the int is too long on ia32 builds. :-( http://codereview.chromium.org/7089002/diff/5002/src/objects.cc#newcode6951 src/objects.cc:6951: elems->set(i, obj, UPDATE_WRITE_BARRIER); On 2011/06/06 07:58:23, Mads Ager wrote:
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.
It doesn't really make sense to use GetWriteBarrierMode here, because it needs an AssertNoAllocation object, which would have to e inside the loop after the allocation anyway, so it doesn't save any write barrier operations. We could also test elems for presence in NewSpace explicitly to avoid the write barrier explicitly, but that seems like a case of premature optimization without hard evidence that it's a bottleneck. http://codereview.chromium.org/7089002/diff/5002/src/objects.cc#newcode7802 src/objects.cc:7802: if (!value_is_smi && On 2011/06/06 07:58:23, Mads Ager wrote:
if (!value->IsNumber()) which covers IsSmi() and IsHeapNumber().
Done. http://codereview.chromium.org/7089002/diff/5002/src/objects.cc#newcode7807 src/objects.cc:7807: SetFastElementsCapacityAndLength(elms_length, elms_length); On 2011/06/06 07:58:23, Mads Ager wrote:
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? Done. http://codereview.chromium.org/7089002/diff/5002/src/objects.cc#newcode7817 src/objects.cc:7817: // Check whether there is extra space in fixed array.. On 2011/06/06 07:58:23, Mads Ager wrote:
.. -> .
in the fixed array?
Done. 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 On 2011/06/06 07:58:23, Mads Ager wrote:
Period at end of comment.
Done. 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 On 2011/06/06 07:58:23, Mads Ager wrote:
following
Done. http://codereview.chromium.org/7089002/diff/5002/src/objects.h#newcode2143 src/objects.h:2143: static uint64_t kCanonoicalNonHoleNanLower32; On 2011/06/06 07:58:23, Mads Ager wrote:
Canonical.
Done. http://codereview.chromium.org/7089002/diff/5002/src/objects.h#newcode2144 src/objects.h:2144: static uint64_t kCanonoicalNonHoleNanInt64; On 2011/06/06 07:58:23, Mads Ager wrote:
Canonical.
Done. http://codereview.chromium.org/7089002/diff/5002/src/objects.h#newcode2163 src/objects.h:2163: // Length is smi tagged when it is stored. On 2011/06/06 07:58:23, Mads Ager wrote:
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.
Done. http://codereview.chromium.org/7089002/diff/5002/src/objects.h#newcode3991 src/objects.h:3991: // descriptors and the fast elements bit set. On 2011/06/06 07:58:23, Mads Ager wrote:
These comments should be updated? The fast elements bit is no longer a
bit but a
complete elements type, right?
Done. 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; On 2011/06/06 07:58:23, Mads Ager wrote:
i + 0.5; ?
Done. 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]); On 2011/06/06 07:58:23, Mads Ager wrote:
Ditto.
Done. http://codereview.chromium.org/7089002/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
