Create a common subclass for arrays

Create a common base class for Fixed-, FixedDouble- and ExternalArrays.

Also unify Crankshaft code to load array length.

BUG=v8:1493
TEST=external-arrays.js

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

[danno, 2011-08-10 13:19:58]

Please reviews.

[Kevin Millikin (Chromium), 2011-08-11 08:13:06]

LGTM.

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

http://codereview.chromium.org/7600025/diff/7008/src/arm/stub-cache-arm.cc#ne...
src/arm/stub-cache-arm.cc:3817: __ JumpIfNotSmi(key, &miss_force_generic);
Moving this check first slightly speeds up the miss case by avoiding the
elements load.

http://codereview.chromium.org/7600025/diff/7008/src/arm/stub-cache-arm.cc#ne...
src/arm/stub-cache-arm.cc:3820: __ SmiUntag(r4, key);
If you push the untagging down to all the current uses of r4, I think most of
them can avoid the untagging.  (Maybe only the EXTERNAL_FLOAT_ELEMENTS case
can't?)

http://codereview.chromium.org/7600025/diff/7008/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/7600025/diff/7008/src/ia32/stub-cache-ia32.cc#...
src/ia32/stub-cache-ia32.cc:3576: __ mov(ebx, eax);  // Preserve the value in
ebx.
I don't understand the comment anymore.  The value in ebx is immediately
changed.  I think it's obvious that this move is to avoid destroying eax by the
untag, so I'd focus on why:

// Preserve the value in eax as the return value.

http://codereview.chromium.org/7600025/diff/7008/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7600025/diff/7008/src/objects.h#newcode65
src/objects.h:65: //       - FixedArrayBase
I don't like this name, because FixedArray is one of the subclasses.  The sane
thing is to call the base class "FixedArray" and the current FixedArray class
"ObjectArray" (because it contains Objects as elements).  And FixedDoubleArray
could become DoubleArray.

There are some problems with that suggestion.  "Object" is already overloaded to
the point of confusion.  To a much lesser extent, the V8 devs are used to a
specific meaning of "FixedArray".

I can't think of anything better.

http://codereview.chromium.org/7600025/diff/7008/src/objects.h#newcode3143
src/objects.h:3143: POINTER_SIZE_ALIGN(kLengthOffset + kPointerSize);
Hmmm.  Can you write FixedArrayBase::kLengthOffset to be clear where the length
is coming from?

[danno, 2011-08-11 13:59:37]

feedback addressed. landing.

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

http://codereview.chromium.org/7600025/diff/7008/src/arm/stub-cache-arm.cc#ne...
src/arm/stub-cache-arm.cc:3817: __ JumpIfNotSmi(key, &miss_force_generic);
On 2011/08/11 08:13:06, Kevin Millikin wrote:
> Moving this check first slightly speeds up the miss case by avoiding the
> elements load.

Done.

http://codereview.chromium.org/7600025/diff/7008/src/arm/stub-cache-arm.cc#ne...
src/arm/stub-cache-arm.cc:3820: __ SmiUntag(r4, key);
On 2011/08/11 08:13:06, Kevin Millikin wrote:
> If you push the untagging down to all the current uses of r4, I think most of
> them can avoid the untagging.  (Maybe only the EXTERNAL_FLOAT_ELEMENTS case
> can't?)

Done.

http://codereview.chromium.org/7600025/diff/7008/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/7600025/diff/7008/src/ia32/stub-cache-ia32.cc#...
src/ia32/stub-cache-ia32.cc:3576: __ mov(ebx, eax);  // Preserve the value in
ebx.
On 2011/08/11 08:13:06, Kevin Millikin wrote:
> I don't understand the comment anymore.  The value in ebx is immediately
> changed.  I think it's obvious that this move is to avoid destroying eax by
the
> untag, so I'd focus on why:
> 
> // Preserve the value in eax as the return value.

Done.

http://codereview.chromium.org/7600025/diff/7008/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7600025/diff/7008/src/objects.h#newcode65
src/objects.h:65: //       - FixedArrayBase
I hate the name, too, but I'd like to fix that in another CL.

On 2011/08/11 08:13:06, Kevin Millikin wrote:
> I don't like this name, because FixedArray is one of the subclasses.  The sane
> thing is to call the base class "FixedArray" and the current FixedArray class
> "ObjectArray" (because it contains Objects as elements).  And FixedDoubleArray
> could become DoubleArray.
> 
> There are some problems with that suggestion.  "Object" is already overloaded
to
> the point of confusion.  To a much lesser extent, the V8 devs are used to a
> specific meaning of "FixedArray".
> 
> I can't think of anything better.

http://codereview.chromium.org/7600025/diff/7008/src/objects.h#newcode3143
src/objects.h:3143: POINTER_SIZE_ALIGN(kLengthOffset + kPointerSize);
On 2011/08/11 08:13:06, Kevin Millikin wrote:
> Hmmm.  Can you write FixedArrayBase::kLengthOffset to be clear where the
length
> is coming from?

Done.