Implement core support for FixedDoubleArrays.

Implement core support for FixedDoubleArrays.

Under a flag without IC or Crankshaft support.

BUG=none
TEST=none

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

[danno, 2011-06-03 13:42:17]

Please review.

[Mads Ager (chromium), 2011-06-06 07:58:23]

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-ar...
File test/mjsunit/unbox-double-arrays.js (right):

http://codereview.chromium.org/7089002/diff/5002/test/mjsunit/unbox-double-ar...
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-ar...
test/mjsunit/unbox-double-arrays.js:39: assertEquals(i+.5, foo[i]);
Ditto.

[danno, 2011-06-08 12:09:42]

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-ar...
File test/mjsunit/unbox-double-arrays.js (right):

http://codereview.chromium.org/7089002/diff/5002/test/mjsunit/unbox-double-ar...
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-ar...
test/mjsunit/unbox-double-arrays.js:39: assertEquals(i+.5, foo[i]);
On 2011/06/06 07:58:23, Mads Ager wrote:
> Ditto.

Done.

[Vyacheslav Egorov (Chromium), 2011-06-08 12:31:16]

GC DBC

http://codereview.chromium.org/7089002/diff/17001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7089002/diff/17001/src/heap.cc#newcode1310
src/heap.cc:1310: table_.Register(kVisitByteArray, &EvacuateByteArray);
If FixedDoubleArray can be in the new space you need to register specialization
here.

http://codereview.chromium.org/7089002/diff/17001/src/objects-visiting.h
File src/objects-visiting.h (right):

http://codereview.chromium.org/7089002/diff/17001/src/objects-visiting.h#newc...
src/objects-visiting.h:294: table_.Register(kVisitByteArray, &VisitByteArray);
If FixedDoubleArray can be in a new space you need to register specialization
here.

[Mads Ager (chromium), 2011-06-08 13:13:13]

This is looking good. Once Slava's comments are addressed I think you can land
this under the flag.

My main comment is that I think there is stuff missing for double array elements
at this point. Off the top of my head I can think of AddKeysFromJSArray,
HasElementPostInterceptor, EstimateElementsCount... I think we need to go
through all the places where FAST_ELEMENTS are used, add a test and make it
pass.

Since this is under a flag, you can do this in a separate change list to keep
this one small.

http://codereview.chromium.org/7089002/diff/17001/src/factory.h
File src/factory.h (right):

http://codereview.chromium.org/7089002/diff/17001/src/factory.h#newcode52
src/factory.h:52: // Allocate a new fixed double array with undefined entries.
Nit: Could you update the comment to use the wording: uninitialized fixed double
array? And do the same for NewFixedArray above? I get confused by the use of
undefined here (not sure if it means the 'undefined' value).

http://codereview.chromium.org/7089002/diff/17001/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/7089002/diff/17001/src/objects-inl.h#newcode1627
src/objects-inl.h:1627: ASSERT(map() != HEAP->fixed_cow_array_map());
If you assert for fixed_cow_array_map should you assert for normal
fixed_array_map too?

http://codereview.chromium.org/7089002/diff/17001/src/objects-inl.h#newcode1635
src/objects-inl.h:1635: ASSERT(map() != HEAP->fixed_cow_array_map());
Ditto?

http://codereview.chromium.org/7089002/diff/17001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7089002/diff/17001/src/objects.cc#newcode8051
src/objects.cc:8051: // Check whether there is extra space in the fixed array..
.. -> .

http://codereview.chromium.org/7089002/diff/17001/src/objects.cc#newcode8452
src/objects.cc:8452: return GetHeap()->AllocateHeapNumber(double_value);
Use NumberFromDouble?

[danno, 2011-06-09 09:45:39]

feedback addressed, landing...

http://codereview.chromium.org/7089002/diff/17001/src/factory.h
File src/factory.h (right):

http://codereview.chromium.org/7089002/diff/17001/src/factory.h#newcode52
src/factory.h:52: // Allocate a new fixed double array with undefined entries.
On 2011/06/08 13:13:13, Mads Ager wrote:
> Nit: Could you update the comment to use the wording: uninitialized fixed
double
> array? And do the same for NewFixedArray above? I get confused by the use of
> undefined here (not sure if it means the 'undefined' value).

Done.

http://codereview.chromium.org/7089002/diff/17001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7089002/diff/17001/src/heap.cc#newcode1310
src/heap.cc:1310: table_.Register(kVisitByteArray, &EvacuateByteArray);
On 2011/06/08 12:31:16, Vyacheslav Egorov wrote:
> If FixedDoubleArray can be in the new space you need to register
specialization
> here.

Done.

http://codereview.chromium.org/7089002/diff/17001/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/7089002/diff/17001/src/objects-inl.h#newcode1627
src/objects-inl.h:1627: ASSERT(map() != HEAP->fixed_cow_array_map());
On 2011/06/08 13:13:13, Mads Ager wrote:
> If you assert for fixed_cow_array_map should you assert for normal
> fixed_array_map too?

Done.

http://codereview.chromium.org/7089002/diff/17001/src/objects-inl.h#newcode1635
src/objects-inl.h:1635: ASSERT(map() != HEAP->fixed_cow_array_map());
On 2011/06/08 13:13:13, Mads Ager wrote:
> Ditto?

Done.

http://codereview.chromium.org/7089002/diff/17001/src/objects-visiting.h
File src/objects-visiting.h (right):

http://codereview.chromium.org/7089002/diff/17001/src/objects-visiting.h#newc...
src/objects-visiting.h:294: table_.Register(kVisitByteArray, &VisitByteArray);
On 2011/06/08 12:31:16, Vyacheslav Egorov wrote:
> If FixedDoubleArray can be in a new space you need to register specialization
> here.

Done.

http://codereview.chromium.org/7089002/diff/17001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7089002/diff/17001/src/objects.cc#newcode8051
src/objects.cc:8051: // Check whether there is extra space in the fixed array..
On 2011/06/08 13:13:13, Mads Ager wrote:
> .. -> .

Done.

http://codereview.chromium.org/7089002/diff/17001/src/objects.cc#newcode8452
src/objects.cc:8452: return GetHeap()->AllocateHeapNumber(double_value);
On 2011/06/08 13:13:13, Mads Ager wrote:
> Use NumberFromDouble?

Done.