Combine typed and pixel arrays.

Combine typed and pixel arrays.

Worth mentioning:
- Specialized versions of pixel array and store/loads inside the generic stubs have been removed, since to have parity for all external arrays, 8 different versions would have to be inlined/checked.
- There's a new constant in v8.h for external arrays with pixel array elements.

[danno, 2011-02-21 13:03:09]

Not quite done yet (map handling on receiver is not quite right), but please
take a look. Thanks.

http://codereview.chromium.org/6546036/diff/46/src/api.cc
File src/api.cc (right):

http://codereview.chromium.org/6546036/diff/46/src/api.cc#newcode2774
src/api.cc:2774: i::Factory::GetExternalPixelArrayElementsMap(
TODO: This needs to be unified with the elements array map generation. Also, a
copy of the map needs to be made if the element type changes (rather than
reusing the slow map), because it invalidates assumptions about the generated
code keyed on the receiver's map.

[Mads Ager (chromium), 2011-02-21 13:32:27]

Looks good. If you finish this, I will take care of the two scary TODOs.

http://codereview.chromium.org/6546036/diff/46/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/6546036/diff/46/src/arm/stub-cache-arm.cc#newc...
src/arm/stub-cache-arm.cc:3707: // Float to pixel conversion is only implemented
in the runtime for now.
Float -> Double?

http://codereview.chromium.org/6546036/diff/46/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/6546036/diff/46/src/objects.cc#newcode7272
src/objects.cc:7272: // TODO(iposva): Add testcase and implement.
Scary. We should do this.

http://codereview.chromium.org/6546036/diff/46/src/objects.cc#newcode7283
src/objects.cc:7283: // TODO(kbr): Add testcase and implement.
Scary. We should do this.

[danno, 2011-03-07 14:58:57]

Kevin, can you please take the review over from Mads in his absence? Thanks.

[Kevin Millikin (Chromium), 2011-03-07 15:41:44]

I've got some small questions and suggestions below.  All else LGTM.

http://codereview.chromium.org/6546036/diff/10001/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

http://codereview.chromium.org/6546036/diff/10001/src/arm/lithium-arm.h#newco...
src/arm/lithium-arm.h:1001: DECLARE_CONCRETE_INSTRUCTION(ExternalArrayLength,
"pixel-array-length")
The string "pixel-array-length" should change to "external-array-length".

http://codereview.chromium.org/6546036/diff/10001/src/factory.cc
File src/factory.cc (right):

http://codereview.chromium.org/6546036/diff/10001/src/factory.cc#newcode253
src/factory.cc:253: Handle<ExternalPixelArray> Factory::NewExternalPixelArray(
Is there a reason not to combine this with Factory::NewExternalArray?

http://codereview.chromium.org/6546036/diff/10001/src/handles.cc
File src/handles.cc (right):

http://codereview.chromium.org/6546036/diff/10001/src/handles.cc#newcode449
src/handles.cc:449: ASSERT(!object->HasExternalPixelElements());
Doesn't the next assert cover this case?

http://codereview.chromium.org/6546036/diff/10001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/6546036/diff/10001/src/heap.cc#newcode2696
src/heap.cc:2696: MaybeObject* Heap::AllocateExternalPixelArray(int length,
Likewise, can you remove this function and use Heap::AllocateExternalArray
instead?

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

http://codereview.chromium.org/6546036/diff/10001/src/objects-inl.h#newcode3497
src/objects-inl.h:3497: UNREACHABLE();
Hmm.  Maybe you should get rid of the default from the last switch and move the
assert that it's EXTERNAL_FLOAT_ARRAY_TYPE and the return here.

http://codereview.chromium.org/6546036/diff/10001/src/objects-inl.h#newcode3515
src/objects-inl.h:3515: return (HasExternalByteElements() ||
Each one of these tests is potentially calling:

map->has_fast_elements();
elements()->IsFixedArray();
elements()->IsExternalArray();
switch on elements()->map()->instance_type();

I wonder if you could just write something like:

return elements()->map()->instance_type() >= FIRST_EXTERNAL_ARRAY_TYPE
  && elements()->map()->instance_type() <= LAST_EXTERNAL_ARRAY_TYPE;

Or even:

return elements()->IsExternalArray();

http://codereview.chromium.org/6546036/diff/10001/src/objects-inl.h#newcode3527
src/objects-inl.h:3527: return GetElementsKind() == EXTERNAL_PIXEL_ELEMENTS;
Likewise, would

return elements()->map()->instance_type() == EXTERNAL_PIXEL_ARRAY_TYPE;

work here?

[danno, 2011-03-08 10:01:56]

addressed feedback.

http://codereview.chromium.org/6546036/diff/10001/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

http://codereview.chromium.org/6546036/diff/10001/src/arm/lithium-arm.h#newco...
src/arm/lithium-arm.h:1001: DECLARE_CONCRETE_INSTRUCTION(ExternalArrayLength,
"pixel-array-length")
On 2011/03/07 15:41:45, Kevin Millikin wrote:
> The string "pixel-array-length" should change to "external-array-length".

Done.

http://codereview.chromium.org/6546036/diff/10001/src/factory.cc
File src/factory.cc (right):

http://codereview.chromium.org/6546036/diff/10001/src/factory.cc#newcode253
src/factory.cc:253: Handle<ExternalPixelArray> Factory::NewExternalPixelArray(
On 2011/03/07 15:41:45, Kevin Millikin wrote:
> Is there a reason not to combine this with Factory::NewExternalArray?

Done.

http://codereview.chromium.org/6546036/diff/10001/src/handles.cc
File src/handles.cc (right):

http://codereview.chromium.org/6546036/diff/10001/src/handles.cc#newcode449
src/handles.cc:449: ASSERT(!object->HasExternalPixelElements());
On 2011/03/07 15:41:45, Kevin Millikin wrote:
> Doesn't the next assert cover this case?

Done.

http://codereview.chromium.org/6546036/diff/10001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/6546036/diff/10001/src/heap.cc#newcode2696
src/heap.cc:2696: MaybeObject* Heap::AllocateExternalPixelArray(int length,
On 2011/03/07 15:41:45, Kevin Millikin wrote:
> Likewise, can you remove this function and use Heap::AllocateExternalArray
> instead?

Done.

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

http://codereview.chromium.org/6546036/diff/10001/src/objects-inl.h#newcode3497
src/objects-inl.h:3497: UNREACHABLE();
On 2011/03/07 15:41:45, Kevin Millikin wrote:
> Hmm.  Maybe you should get rid of the default from the last switch and move
the
> assert that it's EXTERNAL_FLOAT_ARRAY_TYPE and the return here.

Done.

http://codereview.chromium.org/6546036/diff/10001/src/objects-inl.h#newcode3515
src/objects-inl.h:3515: return (HasExternalByteElements() ||
On 2011/03/07 15:41:45, Kevin Millikin wrote:
> Each one of these tests is potentially calling:
> 
> map->has_fast_elements();
> elements()->IsFixedArray();
> elements()->IsExternalArray();
> switch on elements()->map()->instance_type();
> 
> I wonder if you could just write something like:
> 
> return elements()->map()->instance_type() >= FIRST_EXTERNAL_ARRAY_TYPE
>   && elements()->map()->instance_type() <= LAST_EXTERNAL_ARRAY_TYPE;
> 
> Or even:
> 
> return elements()->IsExternalArray();

Done.

http://codereview.chromium.org/6546036/diff/10001/src/objects-inl.h#newcode3527
src/objects-inl.h:3527: return GetElementsKind() == EXTERNAL_PIXEL_ELEMENTS;
On 2011/03/07 15:41:45, Kevin Millikin wrote:
> Likewise, would
> 
> return elements()->map()->instance_type() == EXTERNAL_PIXEL_ARRAY_TYPE;
> 
> work here?

Done.

[Kevin Millikin (Chromium), 2011-03-08 14:26:49]

LGTM.

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

http://codereview.chromium.org/6546036/diff/9192/src/objects-inl.h#newcode3513
src/objects-inl.h:3513: ASSERT(array);
V8 style seems to be ASSERT(array != NULL);