Implement in-heap backing store for typed arrays.

Implement in-heap backing store for typed arrays.

This adds a fixed array sub-type that will represent a backing store for
typed arrays allocated with TypedArray(length) construtor.

R=mvstanton@chromium.org, verwaest@chromium.org

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

[Dmitry Lomov (no reviews), 2013-12-19 16:37:22]

Michael, Toon,
please take a preliminary look.

No platform ports yet.

[mvstanton, 2013-12-20 11:30:51]

It looks pretty straightforward. 

Kind of a nit, it would be neat if the notion of external array and fixed array
could be unified for some checks. It's a little depressing to just add another
check to if() statements all over the code. Maybe predicates like
external_or_fixed() could live in appropriate places, like hydrogen-instructions
and elements-kind.

[Toon Verwaest, 2013-12-23 10:40:32]

https://codereview.chromium.org/101413006/diff/20001/src/elements-kind.h
File src/elements-kind.h (right):

https://codereview.chromium.org/101413006/diff/20001/src/elements-kind.h#newc...
src/elements-kind.h:151: IsFixedFloatOrDoubleElementsKind(kind);
What about just renaming this to IsFixedFloatElementsKind, given that now you
called them both Float[32|64]?

https://codereview.chromium.org/101413006/diff/20001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/101413006/diff/20001/src/heap.cc#newcode2827
src/heap.cc:2827: if (!maybe_obj->ToObject(&obj)) return false;
Can we already change all these ToObject to To, only declare maybe_obj once; and
remove all the { ... }? And probably make "obj" instead Map* map; so that we
don't need to Map::cast obj. Hence:

Map* map;
MaybeObject* maybe_map;

...
maybe_map = AllocateMap(FIXED_INT8_ARRAY_TYPE, kVariableSizeSentinel);
if (!maybe_map->To(&map)) return false;
set_fixed_int8_array_map(map);
...

I think all this syntactical noise is getting more and more ridiculous.

https://codereview.chromium.org/101413006/diff/20001/src/heap.cc#newcode4349
src/heap.cc:4349: int element_size = 1;  // Bogus.
Doesn't it work to set element_size and elements_kind in the UNREACHABLE()
branch above, rather than initialize them here? Then you don't need those //
Bogus labels here and it's in sync with what most other code does.

https://codereview.chromium.org/101413006/diff/20001/src/heap.cc#newcode4356
src/heap.cc:4356: { MaybeObject* maybe_object = AllocateRaw(size, space,
OLD_DATA_SPACE);
Remove the { ... }.

https://codereview.chromium.org/101413006/diff/20001/src/heap.cc#newcode4357
src/heap.cc:4357: if (!maybe_object->To<HeapObject>(&object)) return
maybe_object;
Is the <HeapObject> really necessary? If not remove it. Otherwise, if it's
necessary cause the map isn't correct yet, can we directly cast to
FixedTypedArrayBase?

https://codereview.chromium.org/101413006/diff/20001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/101413006/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:6441: SetGVNFlag(kDependsOnArrayElements);
Don't you want to make a separate DependsOnTypedArrayElements?

https://codereview.chromium.org/101413006/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:6728: ? Representation::Integer32()
4-space indent if you put the ? on a new line.

https://codereview.chromium.org/101413006/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/101413006/diff/20001/src/hydrogen.cc#newcode2040
src/hydrogen.cc:2040: && !IsFixedTypedArrayElementsKind(elements_kind))
&& on previous line. Align new line 1 space further than is currently the case.

https://codereview.chromium.org/101413006/diff/20001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/101413006/diff/20001/src/ia32/lithium-ia32.cc...
src/ia32/lithium-ia32.cc:2358: instr->elements()->representation().IsTagged())
||
Align the instr at the same level of indentation.

https://codereview.chromium.org/101413006/diff/20001/src/ia32/lithium-ia32.cc...
src/ia32/lithium-ia32.cc:2369: return new(zone()) LStoreKeyed(backing_store,
I'd put backing_store, key, val); on a single line.

https://codereview.chromium.org/101413006/diff/20001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/101413006/diff/20001/src/objects-debug.cc#new...
src/objects-debug.cc:340: && HeapObject::cast(this)->map()->instance_type()
&& on previous line. Also ==. Indent +4 spaces for the line after ==.

https://codereview.chromium.org/101413006/diff/20001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/101413006/diff/20001/src/objects-inl.h#newcod...
src/objects-inl.h:145: || IsFixedTypedArrayBase() || IsExternalArray();
|| on previous line.

https://codereview.chromium.org/101413006/diff/20001/src/objects-inl.h#newcod...
src/objects-inl.h:274: || IsFixedTypedArrayBase();
|| on previous line.

https://codereview.chromium.org/101413006/diff/20001/src/objects-inl.h#newcod...
src/objects-inl.h:3573: // Clamp undefined to zero (default). All other types
have been
I'd just write "Clamp undefined to the default value"; rather than specifying
here that that's zero.

https://codereview.chromium.org/101413006/diff/20001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/101413006/diff/20001/src/objects-printer.cc#n...
src/objects-printer.cc:407: }
Nice cleanup!

https://codereview.chromium.org/101413006/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/101413006/diff/20001/src/objects.h#newcode4914
src/objects.h:4914: 
Double newlines between classes.
This is a lot of boilerplate code...

https://codereview.chromium.org/101413006/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/101413006/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:16186: factory->NewFixedTypedArray(kElementCount,
Join lines

https://codereview.chromium.org/101413006/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:16194: }
From what I can tell, out-of-bounds accesses are tested in
ObjectWithExternalArrayTestHelper?

https://codereview.chromium.org/101413006/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:16205: jsobj,
join lines

https://codereview.chromium.org/101413006/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:16211: context.local(),
Don't all these arguments fit on a single line?

[Dmitry Lomov (no reviews), 2014-01-07 15:48:42]

Thanks for review! Comments addressed, proceeding with platform ports

https://codereview.chromium.org/101413006/diff/20001/src/elements-kind.h
File src/elements-kind.h (right):

https://codereview.chromium.org/101413006/diff/20001/src/elements-kind.h#newc...
src/elements-kind.h:151: IsFixedFloatOrDoubleElementsKind(kind);
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> What about just renaming this to IsFixedFloatElementsKind, given that now you
> called them both Float[32|64]?

Done.

https://codereview.chromium.org/101413006/diff/20001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/101413006/diff/20001/src/heap.cc#newcode2827
src/heap.cc:2827: if (!maybe_obj->ToObject(&obj)) return false;
Done (+ more boilerplate scratching)
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> Can we already change all these ToObject to To, only declare maybe_obj once;
and
> remove all the { ... }? And probably make "obj" instead Map* map; so that we
> don't need to Map::cast obj. Hence:
> 
> Map* map;
> MaybeObject* maybe_map;
> 
> ...
> maybe_map = AllocateMap(FIXED_INT8_ARRAY_TYPE, kVariableSizeSentinel);
> if (!maybe_map->To(&map)) return false;
> set_fixed_int8_array_map(map);
> ...
> 
> I think all this syntactical noise is getting more and more ridiculous.

https://codereview.chromium.org/101413006/diff/20001/src/heap.cc#newcode4349
src/heap.cc:4349: int element_size = 1;  // Bogus.
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> Doesn't it work to set element_size and elements_kind in the UNREACHABLE()
> branch above, rather than initialize them here? Then you don't need those //
> Bogus labels here and it's in sync with what most other code does.

Done.

https://codereview.chromium.org/101413006/diff/20001/src/heap.cc#newcode4356
src/heap.cc:4356: { MaybeObject* maybe_object = AllocateRaw(size, space,
OLD_DATA_SPACE);
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> Remove the { ... }.

Done.

https://codereview.chromium.org/101413006/diff/20001/src/heap.cc#newcode4357
src/heap.cc:4357: if (!maybe_object->To<HeapObject>(&object)) return
maybe_object;
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> Is the <HeapObject> really necessary? If not remove it. Otherwise, if it's
> necessary cause the map isn't correct yet, can we directly cast to
> FixedTypedArrayBase?
No because FixedTypedArrayBase::cast checks map as well.

https://codereview.chromium.org/101413006/diff/20001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/101413006/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:6441: SetGVNFlag(kDependsOnArrayElements);
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> Don't you want to make a separate DependsOnTypedArrayElements?

Done. Good suggestion.

https://codereview.chromium.org/101413006/diff/20001/src/hydrogen-instruction...
src/hydrogen-instructions.h:6728: ? Representation::Integer32()
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> 4-space indent if you put the ? on a new line.

Done.

https://codereview.chromium.org/101413006/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/101413006/diff/20001/src/hydrogen.cc#newcode2040
src/hydrogen.cc:2040: && !IsFixedTypedArrayElementsKind(elements_kind))
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> && on previous line. Align new line 1 space further than is currently the
case.

Done.

https://codereview.chromium.org/101413006/diff/20001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/101413006/diff/20001/src/ia32/lithium-ia32.cc...
src/ia32/lithium-ia32.cc:2358: instr->elements()->representation().IsTagged())
||
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> Align the instr at the same level of indentation.

Done.

https://codereview.chromium.org/101413006/diff/20001/src/ia32/lithium-ia32.cc...
src/ia32/lithium-ia32.cc:2369: return new(zone()) LStoreKeyed(backing_store,
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> I'd put backing_store, key, val); on a single line.

Done.

https://codereview.chromium.org/101413006/diff/20001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/101413006/diff/20001/src/objects-debug.cc#new...
src/objects-debug.cc:340: && HeapObject::cast(this)->map()->instance_type()
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> && on previous line. Also ==. Indent +4 spaces for the line after ==.

Done.

https://codereview.chromium.org/101413006/diff/20001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/101413006/diff/20001/src/objects-inl.h#newcod...
src/objects-inl.h:145: || IsFixedTypedArrayBase() || IsExternalArray();
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> || on previous line.

Done.

https://codereview.chromium.org/101413006/diff/20001/src/objects-inl.h#newcod...
src/objects-inl.h:274: || IsFixedTypedArrayBase();
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> || on previous line.

Done.

https://codereview.chromium.org/101413006/diff/20001/src/objects-inl.h#newcod...
src/objects-inl.h:3573: // Clamp undefined to zero (default). All other types
have been
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> I'd just write "Clamp undefined to the default value"; rather than specifying
> here that that's zero.

Done.

https://codereview.chromium.org/101413006/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/101413006/diff/20001/src/objects.h#newcode4914
src/objects.h:4914: 
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> Double newlines between classes.
> This is a lot of boilerplate code...

Done.

https://codereview.chromium.org/101413006/diff/20001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/101413006/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:16186: factory->NewFixedTypedArray(kElementCount,
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> Join lines

Done.

https://codereview.chromium.org/101413006/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:16194: }
Yes. 'set' just asserts on OOB
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> From what I can tell, out-of-bounds accesses are tested in
> ObjectWithExternalArrayTestHelper?

https://codereview.chromium.org/101413006/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:16205: jsobj,
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> join lines

Done.

https://codereview.chromium.org/101413006/diff/20001/test/cctest/test-api.cc#...
test/cctest/test-api.cc:16211: context.local(),
On 2013/12/23 10:40:32, Toon Verwaest wrote:
> Don't all these arguments fit on a single line?

Done.

[Dmitry Lomov (no reviews), 2014-01-14 08:04:11]

Platform ports finished, PTAL

[Toon Verwaest, 2014-01-16 10:54:50]

Looks good from my side minus some nits below.

https://codereview.chromium.org/101413006/diff/400001/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

https://codereview.chromium.org/101413006/diff/400001/src/arm/lithium-arm.cc#...
src/arm/lithium-arm.cc:2145:
!(IsDoubleOrFloatElementsKind(instr->elements_kind()))) ||
remove () around IsDouble...

https://codereview.chromium.org/101413006/diff/400001/src/arm/lithium-arm.cc#...
src/arm/lithium-arm.cc:2147:
(IsDoubleOrFloatElementsKind(instr->elements_kind()))));
Remove () around IsDouble...

https://codereview.chromium.org/101413006/diff/400001/src/arm/lithium-arm.cc#...
src/arm/lithium-arm.cc:2156: (elements_kind == EXTERNAL_UNSIGNED_INT_ELEMENTS)
||
remove ()

https://codereview.chromium.org/101413006/diff/400001/src/arm/lithium-codegen...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/101413006/diff/400001/src/arm/lithium-codegen...
src/arm/lithium-codegen-arm.cc:3374: if (instr->is_external() ||
instr->is_fixed_typed_array()) {
Perhaps we should add a helper method that wraps is_external() ||
is_fixed_typed_array()? Seems like that's used often.

https://codereview.chromium.org/101413006/diff/400001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/101413006/diff/400001/src/objects-inl.h#newco...
src/objects-inl.h:501: if (!Object::IsHeapObject())
Use { } if you put return false on a new line. Also ok as
if (!Object::IsHeapObject()) return false;

https://codereview.chromium.org/101413006/diff/400001/src/objects-inl.h#newco...
src/objects-inl.h:2724: HeapObject::cast(object)->map()->instance_type()
== on previous line

https://codereview.chromium.org/101413006/diff/400001/src/objects-visiting.cc
File src/objects-visiting.cc (right):

https://codereview.chromium.org/101413006/diff/400001/src/objects-visiting.cc...
src/objects-visiting.cc:85: 
spurious line addition

[mvstanton, 2014-01-16 11:49:31]

Looks good to me, I like the macros to reduce boilerplate. With Toon's comments
addressed, then lgtm.

[Dmitry Lomov (no reviews), 2014-01-16 13:52:17]

Comments addressed, PTAL

https://codereview.chromium.org/101413006/diff/400001/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

https://codereview.chromium.org/101413006/diff/400001/src/arm/lithium-arm.cc#...
src/arm/lithium-arm.cc:2145:
!(IsDoubleOrFloatElementsKind(instr->elements_kind()))) ||
On 2014/01/16 10:54:51, Toon Verwaest wrote:
> remove () around IsDouble...

Done.

https://codereview.chromium.org/101413006/diff/400001/src/arm/lithium-arm.cc#...
src/arm/lithium-arm.cc:2147:
(IsDoubleOrFloatElementsKind(instr->elements_kind()))));
On 2014/01/16 10:54:51, Toon Verwaest wrote:
> Remove () around IsDouble...

Done.

https://codereview.chromium.org/101413006/diff/400001/src/arm/lithium-arm.cc#...
src/arm/lithium-arm.cc:2156: (elements_kind == EXTERNAL_UNSIGNED_INT_ELEMENTS)
||
On 2014/01/16 10:54:51, Toon Verwaest wrote:
> remove ()

Done.

https://codereview.chromium.org/101413006/diff/400001/src/arm/lithium-codegen...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/101413006/diff/400001/src/arm/lithium-codegen...
src/arm/lithium-codegen-arm.cc:3374: if (instr->is_external() ||
instr->is_fixed_typed_array()) {
On 2014/01/16 10:54:51, Toon Verwaest wrote:
> Perhaps we should add a helper method that wraps is_external() ||
> is_fixed_typed_array()? Seems like that's used often.

Done.

https://codereview.chromium.org/101413006/diff/400001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/101413006/diff/400001/src/objects-inl.h#newco...
src/objects-inl.h:501: if (!Object::IsHeapObject())
On 2014/01/16 10:54:51, Toon Verwaest wrote:
> Use { } if you put return false on a new line. Also ok as
> if (!Object::IsHeapObject()) return false;

Done.

https://codereview.chromium.org/101413006/diff/400001/src/objects-inl.h#newco...
src/objects-inl.h:2724: HeapObject::cast(object)->map()->instance_type()
On 2014/01/16 10:54:51, Toon Verwaest wrote:
> == on previous line

Done.

https://codereview.chromium.org/101413006/diff/400001/src/objects-visiting.cc
File src/objects-visiting.cc (right):

https://codereview.chromium.org/101413006/diff/400001/src/objects-visiting.cc...
src/objects-visiting.cc:85: 
On 2014/01/16 10:54:51, Toon Verwaest wrote:
> spurious line addition

Done.

[Toon Verwaest, 2014-01-16 14:01:03]

lgtm

[Dmitry Lomov (no reviews), 2014-01-16 14:19:12]

Committed patchset #7 manually as r18646 (presubmit successful).

[Dmitry Lomov (no reviews), 2014-01-16 15:01:53]

Committed patchset #7 manually as r18649 (presubmit successful).

[Dmitry Lomov (no reviews), 2014-01-16 16:44:38]

Michael, Toon - PTAL.
I discovered an issue with the patch - I forgot to register new object types
with scavenging visitors.
Unfortunately I've rebased - the interesting files to look at are:

src/heap.cc
src/objects-visiting-inl.h
src/objects-visiting.cc
src/objects-visiting.h

[mvstanton, 2014-01-16 16:52:10]

lgtm

[Dmitry Lomov (no reviews), 2014-01-16 17:08:21]

On 2014/01/16 16:52:10, mvstanton wrote:
> lgtm

Thanks, landing again - third time's the charm

[Dmitry Lomov (no reviews), 2014-01-16 17:09:11]

Committed patchset #8 manually as r18651 (presubmit successful).