Support all fast elements kinds in the major array operations.

Support all fast elements kinds in the major array operations.
Currently missing support for unshift.

BUG=

Committed: https://code.google.com/p/v8/source/detail?r=12969

[Toon Verwaest, 2012-11-13 15:02:01]

PTAL.

[danno, 2012-11-13 22:05:12]

First round of comments.

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

http://codereview.chromium.org/11377132/diff/1/src/arm/stub-cache-arm.cc#newc...
src/arm/stub-cache-arm.cc:1682: 
nit: why the double space?

http://codereview.chromium.org/11377132/diff/1/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode328
src/builtins.cc:328: static void MoveDoubleElements(FixedDoubleArray* dst,
Seems like you should be using the CopyElement routine in the appropriate
ElementsAccessor from elements.cc, or at least calling
CopyDoubleToDoubleElements in elements.cc.

http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode640
src/builtins.cc:640: FillWithHoles(new_elms, len + index, new_length);
Why does this need to be here? Can't you just do it in the case that needs to
grow the array? You know that there will be holes in the destination otherwise.

http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode722
src/builtins.cc:722: Object* first = NULL;
Use ElementsAccessor::Get method here. It does what you need.

http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode841
src/builtins.cc:841: if (array->HasFastSmiOrObjectElements() ||
array->HasFastDoubleElements()) {
Might be worth adding HasFastElements, or calling
IsFastElementsKind(array->elements_kind())

http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode862
src/builtins.cc:862: if (object->HasFastSmiOrObjectElements() ||
Might be worth adding HasFastElements, or calling
IsFastElementsKind(array->elements_kind())

http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode876
src/builtins.cc:876: if (object->HasFastSmiOrObjectElements()) {
I think you can handle this with ElementAccessors a little more elegantly,

ElementsAccessor* accessor = object->GetElementsAccessor();
for (.....) {
  if (!accessor->HasElement(i) ...
}

dont know if its fast enough, tho.

http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode1067
src/builtins.cc:1067: if (IsFastDoubleElementsKind(elements_kind)) {
Use ElementsAccessor for this, it handles the difference cases of the
elements_kind

http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode1208
src/builtins.cc:1208: JSArray::cast(arg)->HasFastDoubleElements()) ||
HasFastElements

http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode1247
src/builtins.cc:1247: if (IsFastSmiElementsKind(elements_kind)) {
You should be able to use ElementsAccessors for all the copying below.

http://codereview.chromium.org/11377132/diff/1/src/builtins.cc#newcode1315
src/builtins.cc:1315: if (array->elements()->IsFixedDoubleArray()) {
Use ElementsAccessors here, too.

http://codereview.chromium.org/11377132/diff/1/src/elements.cc
File src/elements.cc (right):

http://codereview.chromium.org/11377132/diff/1/src/elements.cc#newcode156
src/elements.cc:156: (copy_size + static_cast<int>(from_start)) <=
from->length());
Please handle the hole here and handle the ElementsAccessor::kCopyToEnd and the
ElementsAccessor::kCopyToEndAndInitializeToHole constants properly.

http://codereview.chromium.org/11377132/diff/1/src/elements.cc#newcode183
src/elements.cc:183: if (raw_copy_size ==
ElementsAccessor::kCopyToEndAndInitializeToHole) {
If you take my suggestion from the header file, this code (and similar like it
later) will be moved out of #ifdef debug and made conditional by the
FILL/DONT_FILL enum passed in. in the DONT_FILL case, the assert can remain.

http://codereview.chromium.org/11377132/diff/1/src/elements.h
File src/elements.h (right):

http://codereview.chromium.org/11377132/diff/1/src/elements.h#newcode115
src/elements.h:115: static const int kCopyToEndAndInitializeToHole = -2;
You probably should get rid of this contant above, and instead thread through a
boolean value/enum {FILL_TO_END_WITH_HOLE, DONT_FILL_TO_END_WITH_HOLE} that is
passed as an additional parameter to CopyElements. This would allow your call
sites of CopyXXX that have to post-fill to not have to do so.

http://codereview.chromium.org/11377132/diff/1/src/elements.h#newcode187
src/elements.h:187: MaybeObject* CopyDoubleToObjectElements(FixedDoubleArray*
from_obj,
Do you really need to add these here? Or can you get away with ElementsAccessors
everywhere?

http://codereview.chromium.org/11377132/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/11377132/diff/1/src/heap.cc#newcode4184
src/heap.cc:4184: // ASSERT(JSObject::cast(obj)->HasFastSmiOrObjectElements());
Remove //?

http://codereview.chromium.org/11377132/diff/1/src/heap.cc#newcode4249
src/heap.cc:4249: if (elements_kind == FAST_DOUBLE_ELEMENTS ||
IsFastDoubleElements(elements_kind)

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

http://codereview.chromium.org/11377132/diff/1/src/objects-inl.h#newcode1947
src/objects-inl.h:1947: Object** FixedArrayBase::data_start() {
Whoa. Either return a void* and the caller has to cast appropriately, or this
needs to stay a FixedArray-specific method.

http://codereview.chromium.org/11377132/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/11377132/diff/1/src/parser.cc#newcode3789
src/parser.cc:3789: // Very small array literals that don't have a concrete hint
about their type
Remove all the code from this line (beginning of comment)

http://codereview.chromium.org/11377132/diff/1/src/parser.cc#newcode3796
src/parser.cc:3796: }
... to here.

http://codereview.chromium.org/11377132/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/11377132/diff/1/src/runtime.cc#newcode4106
src/runtime.cc:4106: !IsFastSmiOrObjectElementsKind(elements_kind)) {
Can't you reduce the entire multi-part test above to
IsFastDoubleelementsKind(element_kind)?

[Toon Verwaest, 2012-11-14 11:53:13]

Addressed comments. I left out the enum for holes for now.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

https://chromiumcodereview.appspot.com/11377132/diff/1/src/arm/stub-cache-arm...
src/arm/stub-cache-arm.cc:1682: 
On 2012/11/13 22:05:12, danno wrote:
> nit: why the double space?

Done.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc
File src/builtins.cc (right):

https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcod...
src/builtins.cc:328: static void MoveDoubleElements(FixedDoubleArray* dst,
On 2012/11/13 22:05:12, danno wrote:
> Seems like you should be using the CopyElement routine in the appropriate
> ElementsAccessor from elements.cc, or at least calling
> CopyDoubleToDoubleElements in elements.cc.

Done.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcod...
src/builtins.cc:640: FillWithHoles(new_elms, len + index, new_length);
On 2012/11/13 22:05:12, danno wrote:
> Why does this need to be here? Can't you just do it in the case that needs to
> grow the array? You know that there will be holes in the destination
otherwise.

Done.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcod...
src/builtins.cc:722: Object* first = NULL;
On 2012/11/13 22:05:12, danno wrote:
> Use ElementsAccessor::Get method here. It does what you need.

Done.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcod...
src/builtins.cc:841: if (array->HasFastSmiOrObjectElements() ||
array->HasFastDoubleElements()) {
On 2012/11/13 22:05:12, danno wrote:
> Might be worth adding HasFastElements, or calling
> IsFastElementsKind(array->elements_kind())

I originally wrote the code with int32 arrays in mind; but probably I should add
an ASSERT instead to avoid runtime overhead.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcod...
src/builtins.cc:862: if (object->HasFastSmiOrObjectElements() ||
On 2012/11/13 22:05:12, danno wrote:
> Might be worth adding HasFastElements, or calling
> IsFastElementsKind(array->elements_kind())

Done.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcod...
src/builtins.cc:876: if (object->HasFastSmiOrObjectElements()) {
On 2012/11/13 22:05:12, danno wrote:
> I think you can handle this with ElementAccessors a little more elegantly,
> 
> ElementsAccessor* accessor = object->GetElementsAccessor();
> for (.....) {
>   if (!accessor->HasElement(i) ...
> }
> 
> dont know if its fast enough, tho.

Done.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcod...
src/builtins.cc:1067: if (IsFastDoubleElementsKind(elements_kind)) {
On 2012/11/13 22:05:12, danno wrote:
> Use ElementsAccessor for this, it handles the difference cases of the
> elements_kind

Done.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcod...
src/builtins.cc:1208: JSArray::cast(arg)->HasFastDoubleElements()) ||
On 2012/11/13 22:05:12, danno wrote:
> HasFastElements

Done.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcod...
src/builtins.cc:1247: if (IsFastSmiElementsKind(elements_kind)) {
On 2012/11/13 22:05:12, danno wrote:
> You should be able to use ElementsAccessors for all the copying below.

Done.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/builtins.cc#newcod...
src/builtins.cc:1315: if (array->elements()->IsFixedDoubleArray()) {
On 2012/11/13 22:05:12, danno wrote:
> Use ElementsAccessors here, too.

Done.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/elements.cc
File src/elements.cc (right):

https://chromiumcodereview.appspot.com/11377132/diff/1/src/elements.cc#newcod...
src/elements.cc:156: (copy_size + static_cast<int>(from_start)) <=
from->length());
Removed this function again now that I just use accessors.

On 2012/11/13 22:05:12, danno wrote:
> Please handle the hole here and handle the ElementsAccessor::kCopyToEnd and
the
> ElementsAccessor::kCopyToEndAndInitializeToHole constants properly.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/elements.h
File src/elements.h (right):

https://chromiumcodereview.appspot.com/11377132/diff/1/src/elements.h#newcode187
src/elements.h:187: MaybeObject* CopyDoubleToObjectElements(FixedDoubleArray*
from_obj,
On 2012/11/13 22:05:12, danno wrote:
> Do you really need to add these here? Or can you get away with
ElementsAccessors
> everywhere?

Done.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/heap.cc
File src/heap.cc (right):

https://chromiumcodereview.appspot.com/11377132/diff/1/src/heap.cc#newcode4184
src/heap.cc:4184: // ASSERT(JSObject::cast(obj)->HasFastSmiOrObjectElements());
On 2012/11/13 22:05:12, danno wrote:
> Remove //?

Done.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://chromiumcodereview.appspot.com/11377132/diff/1/src/objects-inl.h#newc...
src/objects-inl.h:1947: Object** FixedArrayBase::data_start() {
On 2012/11/13 22:05:12, danno wrote:
> Whoa. Either return a void* and the caller has to cast appropriately, or this
> needs to stay a FixedArray-specific method.

Done.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/parser.cc
File src/parser.cc (right):

https://chromiumcodereview.appspot.com/11377132/diff/1/src/parser.cc#newcode3789
src/parser.cc:3789: // Very small array literals that don't have a concrete hint
about their type
On 2012/11/13 22:05:12, danno wrote:
> Remove all the code from this line (beginning of comment)

Done.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/parser.cc#newcode3796
src/parser.cc:3796: }
On 2012/11/13 22:05:12, danno wrote:
> ... to here.

Done.

https://chromiumcodereview.appspot.com/11377132/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://chromiumcodereview.appspot.com/11377132/diff/1/src/runtime.cc#newcode...
src/runtime.cc:4106: !IsFastSmiOrObjectElementsKind(elements_kind)) {
On 2012/11/13 22:05:12, danno wrote:
> Can't you reduce the entire multi-part test above to
> IsFastDoubleelementsKind(element_kind)?

Done. Added ASSERT to easily find the place back when adding more raw
elementskinds.

[danno, 2012-11-15 10:34:28]

LGTM with comments.

https://chromiumcodereview.appspot.com/11377132/diff/14003/src/builtins.cc
File src/builtins.cc (right):

https://chromiumcodereview.appspot.com/11377132/diff/14003/src/builtins.cc#ne...
src/builtins.cc:674: // Get top element
Simplify both cases to (if performance allows)

ElementsAccessor* accessor = array->GetElementsAccessor();
int new_length = len - 1;
MaybeObject* result = accessor->Get(array, array, new_length);
if (result->IsFailure()) return result;
MaybeObject* maybe_length = accessor->SetLength(array, new_length);
if (maybe_length->IsFailure()) return maybe_length;
if (!result->IsTheHole()) return result;

https://chromiumcodereview.appspot.com/11377132/diff/14003/src/heap.cc
File src/heap.cc (right):

https://chromiumcodereview.appspot.com/11377132/diff/14003/src/heap.cc#newcod...
src/heap.cc:4250: elements_kind == FAST_HOLEY_DOUBLE_ELEMENTS) {
IsFastDoubleElementsKind(elements_kind)