In-heap small typed arrays

This implements allocating small typed arrays in heap.

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

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

[Dmitry Lomov (no reviews), 2014-02-27 10:08:51]

Michael, Toon, PTAL.
It is a biggish patch :)

[mvstanton, 2014-03-03 10:37:49]

Hi Dmitry, I didn't yet look at hydrogen.cc, but other files look good.

https://codereview.chromium.org/150813004/diff/180001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/150813004/diff/180001/src/flag-definitions.h#...
src/flag-definitions.h:359: "threshold for in-heap typed array")
how about typed_array_max_size_in_heap. 

Also, could you relate this constant to the max object size in the heap with
ASSERTS somewhere appropriate? Obviously, 64 bytes is very small compared to max
object size in new space, but an assert would offer long-lasting protection.

https://codereview.chromium.org/150813004/diff/180001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/150813004/diff/180001/src/objects.cc#newcode1...
src/objects.cc:16457: printf("Materializing buffer!\n");
Remove this printf().

https://codereview.chromium.org/150813004/diff/180001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/150813004/diff/180001/src/runtime.cc#newcode956
src/runtime.cc:956: holder->map()->Print();
Remove this debugging code.

https://codereview.chromium.org/150813004/diff/180001/src/runtime.cc#newcode1063
src/runtime.cc:1063: memcpy(
Could the memcpy code in this block be encapsulated into a JSTypedArray::Copy()
function?

[Dmitry Lomov (no reviews), 2014-03-10 09:20:34]

Comments addressed, please take another look 
(I am sorry, I had to rebase this)

https://codereview.chromium.org/150813004/diff/180001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/150813004/diff/180001/src/flag-definitions.h#...
src/flag-definitions.h:359: "threshold for in-heap typed array")
On 2014/03/03 10:37:49, mvstanton wrote:
> how about typed_array_max_size_in_heap. 
> 
> Also, could you relate this constant to the max object size in the heap with
> ASSERTS somewhere appropriate? Obviously, 64 bytes is very small compared to
max
> object size in new space, but an assert would offer long-lasting protection.

Done.

https://codereview.chromium.org/150813004/diff/180001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/150813004/diff/180001/src/objects.cc#newcode1...
src/objects.cc:16457: printf("Materializing buffer!\n");
On 2014/03/03 10:37:49, mvstanton wrote:
> Remove this printf().

Done.

https://codereview.chromium.org/150813004/diff/180001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/150813004/diff/180001/src/runtime.cc#newcode956
src/runtime.cc:956: holder->map()->Print();
On 2014/03/03 10:37:49, mvstanton wrote:
> Remove this debugging code.

Done.

https://codereview.chromium.org/150813004/diff/180001/src/runtime.cc#newcode1063
src/runtime.cc:1063: memcpy(
On 2014/03/03 10:37:49, mvstanton wrote:
> Could the memcpy code in this block be encapsulated into a
JSTypedArray::Copy()
> function?

Maybe, but maybe in separate CL?

[Toon Verwaest, 2014-03-10 14:15:34]

I added some more superficial comments inline.

My main comment is though: are typed elements kinds at all related to normal
elements transitions?

It seems rather weird to mix up the "IsMoreGeneralElementsTransition" related
code (that puts them in sequence etc) with typed arrays. Typed arrays, as far as
I know, can't transition elements kind. So they don't really belong in sequence
as we don't transition between them. Your changes makes that code more complex
(and slower) without  providing any benefit to that part (If I'm not
mistaken...).

We may want to have dedicated transitions for them though, to ensure that we can
store them all. Maybe we should have internal symbols for each specific
transition so we can add all of them to a same root map; rather than putting
them in sequence?

https://codereview.chromium.org/150813004/diff/200001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/150813004/diff/200001/src/objects.cc#newcode3260
src/objects.cc:3260: IsFastElementsKind(to_kind) ||
IsExternalArrayElementsKind(to_kind)
IsTransitionElementsKind(to_kind)?

https://codereview.chromium.org/150813004/diff/200001/src/objects.cc#newcode3277
src/objects.cc:3277: if (to_kind != kind &&
current_map->HasElementsTransition()) {
Can this only happen for DICTIONARY elements now? If so, can we ASSERT?

https://codereview.chromium.org/150813004/diff/200001/src/objects.cc#newcode3282
src/objects.cc:3282: ASSERT(current_map->elements_kind() == target_kind);
This ASSERT is wrong afaict. If we didnt' have an elements transition above, and
the target kind is DICTIONARY_ELEMENTS, the closest transition will still be
whatever fast kind came right before that.

https://codereview.chromium.org/150813004/diff/200001/src/objects.cc#newcode3324
src/objects.cc:3324: if (kind != to_kind) {
Can this only happen for DICTIONARY elements now? If so, can we ASSERT?

[mvstanton, 2014-03-11 10:43:50]

LGTM. A few comments.

https://codereview.chromium.org/150813004/diff/180001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/150813004/diff/180001/src/runtime.cc#newcode1063
src/runtime.cc:1063: memcpy(
On 2014/03/10 09:20:35, Dmitry Lomov (chromium) wrote:
> On 2014/03/03 10:37:49, mvstanton wrote:
> > Could the memcpy code in this block be encapsulated into a
> JSTypedArray::Copy()
> > function?
> 
> Maybe, but maybe in separate CL?

Yep, okay. It seems weird to me to see primitive memory operations operating on
objects, the instinct is to hide that in implementation.

https://codereview.chromium.org/150813004/diff/200001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/150813004/diff/200001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8466: static Handle<Map> TypedArrayMap(Isolate* isolate,
nit: add another space above this method.

https://codereview.chromium.org/150813004/diff/200001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8612: 
A short comment here to explain what you are doing to compute total_size.

[Dmitry Lomov, 2014-03-11 10:52:13]

On 2014/03/10 14:15:34, Toon Verwaest wrote:
> I added some more superficial comments inline.
> 
> My main comment is though: are typed elements kinds at all related to normal
> elements transitions?
> 
> It seems rather weird to mix up the "IsMoreGeneralElementsTransition" related
> code (that puts them in sequence etc) with typed arrays. Typed arrays, as far
as
> I know, can't transition elements kind. 

But they do transition elements kind. UINT8_ELEMENTS transitions to
EXTERNAL_UINT8_ELEMENTS as soon as you call .buffer on it.
Let's chat offline.

> So they don't really belong in sequence
> as we don't transition between them. Your changes makes that code more complex
> (and slower) without  providing any benefit to that part (If I'm not
> mistaken...).
> 
> We may want to have dedicated transitions for them though, to ensure that we
can
> store them all. Maybe we should have internal symbols for each specific
> transition so we can add all of them to a same root map; rather than putting
> them in sequence?
> 
> https://codereview.chromium.org/150813004/diff/200001/src/objects.cc
> File src/objects.cc (right):
> 
>
https://codereview.chromium.org/150813004/diff/200001/src/objects.cc#newcode3260
> src/objects.cc:3260: IsFastElementsKind(to_kind) ||
> IsExternalArrayElementsKind(to_kind)
> IsTransitionElementsKind(to_kind)?
> 
>
https://codereview.chromium.org/150813004/diff/200001/src/objects.cc#newcode3277
> src/objects.cc:3277: if (to_kind != kind &&
> current_map->HasElementsTransition()) {
> Can this only happen for DICTIONARY elements now? If so, can we ASSERT?
> 
>
https://codereview.chromium.org/150813004/diff/200001/src/objects.cc#newcode3282
> src/objects.cc:3282: ASSERT(current_map->elements_kind() == target_kind);
> This ASSERT is wrong afaict. If we didnt' have an elements transition above,
and
> the target kind is DICTIONARY_ELEMENTS, the closest transition will still be
> whatever fast kind came right before that.
> 
>
https://codereview.chromium.org/150813004/diff/200001/src/objects.cc#newcode3324
> src/objects.cc:3324: if (kind != to_kind) {
> Can this only happen for DICTIONARY elements now? If so, can we ASSERT?

[Dmitry Lomov (no reviews), 2014-03-24 08:31:22]

PTAL (sorry, I had to rebase again)

https://codereview.chromium.org/150813004/diff/200001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/150813004/diff/200001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8466: static Handle<Map> TypedArrayMap(Isolate* isolate,
On 2014/03/11 10:43:51, mvstanton wrote:
> nit: add another space above this method.

Done.

https://codereview.chromium.org/150813004/diff/200001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8612: 
On 2014/03/11 10:43:51, mvstanton wrote:
> A short comment here to explain what you are doing to compute total_size.

Done.

https://codereview.chromium.org/150813004/diff/200001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/150813004/diff/200001/src/objects.cc#newcode3260
src/objects.cc:3260: IsFastElementsKind(to_kind) ||
IsExternalArrayElementsKind(to_kind)
On 2014/03/10 14:15:34, Toon Verwaest wrote:
> IsTransitionElementsKind(to_kind)?
No, this is not IsTransitionElementsKind.

https://codereview.chromium.org/150813004/diff/200001/src/objects.cc#newcode3277
src/objects.cc:3277: if (to_kind != kind &&
current_map->HasElementsTransition()) {
On 2014/03/10 14:15:34, Toon Verwaest wrote:
> Can this only happen for DICTIONARY elements now? If so, can we ASSERT?

Done.

https://codereview.chromium.org/150813004/diff/200001/src/objects.cc#newcode3282
src/objects.cc:3282: ASSERT(current_map->elements_kind() == target_kind);
On 2014/03/10 14:15:34, Toon Verwaest wrote:
> This ASSERT is wrong afaict. If we didnt' have an elements transition above,
and
> the target kind is DICTIONARY_ELEMENTS, the closest transition will still be
> whatever fast kind came right before that.

target_kind is not DICTIONARY_ELEMENTS in that case, target_kind is what we were
trying to reach (see above)

https://codereview.chromium.org/150813004/diff/200001/src/objects.cc#newcode3324
src/objects.cc:3324: if (kind != to_kind) {
On 2014/03/10 14:15:34, Toon Verwaest wrote:
> Can this only happen for DICTIONARY elements now? If so, can we ASSERT?
No, this happens at the end of all chains.

[Toon Verwaest, 2014-03-24 11:16:12]

A few more comments. Would be nice to have that hydrogen code split up a little
bit, given that it's so branchy, and there's not so much shared code.

https://codereview.chromium.org/150813004/diff/220001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/150813004/diff/220001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8583: HType::JSArray(),
Is this correct? I'd think only obj is HType::JSArray(), not the elements.

https://codereview.chromium.org/150813004/diff/220001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8636: total_size->ClearFlag(HValue::kCanOverflow);
Is this the only place where we do alignment? If not, can we join that code into
an alignment helper function?

https://codereview.chromium.org/150813004/diff/220001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8642: Add<HAllocate>(total_size, HType::JSArray(),
Not a JSArray()

[Dmitry Lomov (no reviews), 2014-03-25 10:17:39]

Comments addressed, PTAL

https://codereview.chromium.org/150813004/diff/220001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/150813004/diff/220001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8583: HType::JSArray(),
On 2014/03/24 11:16:12, Toon Verwaest wrote:
> Is this correct? I'd think only obj is HType::JSArray(), not the elements.

Done.

https://codereview.chromium.org/150813004/diff/220001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8636: total_size->ClearFlag(HValue::kCanOverflow);
On 2014/03/24 11:16:12, Toon Verwaest wrote:
> Is this the only place where we do alignment? If not, can we join that code
into
> an alignment helper function?

Done.

https://codereview.chromium.org/150813004/diff/220001/src/hydrogen.cc#newcode...
src/hydrogen.cc:8642: Add<HAllocate>(total_size, HType::JSArray(),
On 2014/03/24 11:16:12, Toon Verwaest wrote:
> Not a JSArray()

Done.

[Toon Verwaest, 2014-03-25 10:49:01]

lgtm

[Dmitry Lomov (no reviews), 2014-03-25 12:52:08]

Committed patchset #5 manually as r20240 (presubmit successful).

[Dmitry Lomov (no reviews), 2014-03-25 13:22:43]

Committed patchset #6 manually as r20244 (presubmit successful).

[Dmitry Lomov (no reviews), 2014-03-26 12:50:34]

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