Implement .forEach() on typed arrays

Implement .forEach() on typed arrays

BUG=v8:3578
LOG=Y

[aperez, 2014-09-18 18:15:37]

aperez@igalia.com changed reviewers:
+ wingo@igalia.com

[aperez, 2014-09-18 18:18:40]

https://codereview.chromium.org/583723002/diff/1/test/mjsunit/harmony/typedar...
File test/mjsunit/harmony/typedarrays-foreach.js (right):

https://codereview.chromium.org/583723002/diff/1/test/mjsunit/harmony/typedar...
test/mjsunit/harmony/typedarrays-foreach.js:39: // Respect holes
I have to remove this leftover comment, typed arrays don't have holes.

[aperez, 2014-09-18 21:27:19]

https://codereview.chromium.org/583723002/diff/20001/src/harmony-typedarray.js
File src/harmony-typedarray.js (right):

https://codereview.chromium.org/583723002/diff/20001/src/harmony-typedarray.j...
src/harmony-typedarray.js:45: if (i in array) {
Also, this check is not needed provided that typed arrays don't have holes. Will
remove.

[wingo, 2014-09-19 10:24:26]

Are there going to be many more of these functions?  If not, it seems overkill
to create a flag and a harmony-foo file.  Dunno.  Otherwise needs some work; see
comments below.

https://codereview.chromium.org/583723002/diff/40001/src/harmony-typedarray.js
File src/harmony-typedarray.js (right):

https://codereview.chromium.org/583723002/diff/40001/src/harmony-typedarray.j...
src/harmony-typedarray.js:1: // Copyright 2013 the V8 project authors. All
rights reserved.
2014

https://codereview.chromium.org/583723002/diff/40001/src/harmony-typedarray.j...
src/harmony-typedarray.js:27: macro TYPED_ARRAY_CONSTRUCTOR(ARRAY_ID, NAME,
ELEMENT_SIZE)
need a link to the spec in a comment:
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%typedarray%.prototy...

https://codereview.chromium.org/583723002/diff/40001/src/harmony-typedarray.j...
src/harmony-typedarray.js:32: var length = ToLength(array.length);
This is incorrect.  The spec says:

%TypedArray%.prototype.indexOf is a distinct function that implements the same
algorithm as Array.prototype.indexOf as defined in 22.1.3.11 except that the
this object’s [[ArrayLength]] internal slot is accessed in place of performing a
[[Get]] of "length".

https://codereview.chromium.org/583723002/diff/40001/src/harmony-typedarray.j...
src/harmony-typedarray.js:40: receiver = ToObject(receiver);
If we have to create a wrapper object, it probably has to be done anew for each
call to "f".  See http://code.google.com/p/v8/issues/detail?id=3536.

[aperez, 2014-09-19 10:38:33]

On 2014/09/19 10:24:26, wingo wrote:
> Are there going to be many more of these functions?  If not, it seems overkill
> to create a flag and a harmony-foo file.  Dunno.  Otherwise needs some work;
see
> comments below.

Yes, I plan to implement other typed array functions that would go into
“harmony-typedarray.js”. If you check the referenced bug, there are about
~20 missing methods in typed arrays, and the plan is to implement them in
small batches, to ease the review process. In this first one CL, I wanted
to only provide one method because there is also modifications to have the
“harmony-typedarray.js” file built into V8.

> https://codereview.chromium.org/583723002/diff/40001/src/harmony-typedarray.js
> File src/harmony-typedarray.js (right):
> 
>
https://codereview.chromium.org/583723002/diff/40001/src/harmony-typedarray.j...
> src/harmony-typedarray.js:1: // Copyright 2013 the V8 project authors. All
> rights reserved.
> 2014

Of course.

>
https://codereview.chromium.org/583723002/diff/40001/src/harmony-typedarray.j...
> src/harmony-typedarray.js:27: macro TYPED_ARRAY_CONSTRUCTOR(ARRAY_ID, NAME,
> ELEMENT_SIZE)
> need a link to the spec in a comment:
>
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%25typedarray%25.pro...
> 
>
https://codereview.chromium.org/583723002/diff/40001/src/harmony-typedarray.j...
> src/harmony-typedarray.js:32: var length = ToLength(array.length);
> This is incorrect.  The spec says:
> 
> %TypedArray%.prototype.indexOf is a distinct function that implements the same
> algorithm as Array.prototype.indexOf as defined in 22.1.3.11 except that the
> this object’s [[ArrayLength]] internal slot is accessed in place of performing
a
> [[Get]] of "length".

Right, then this would use %TypedArrayGetLength(this).

>
https://codereview.chromium.org/583723002/diff/40001/src/harmony-typedarray.j...
> src/harmony-typedarray.js:40: receiver = ToObject(receiver);
> If we have to create a wrapper object, it probably has to be done anew for
each
> call to "f".  See http://code.google.com/p/v8/issues/detail?id=3536.

Certainly.

[aperez, 2014-09-19 15:26:19]

In the latest version of the patch set .forEach() should
behave as per the spec.

Note that the spec says:

  “This function is not generic. If the this value is
   not a object with a [[TypedArrayName]] internal slot,
   a TypeError exception is immediately thrown when this 
   function is called.”

So I have added a check at the top of the function using
“%_ClassOf(this)” to cover this case. Also, this makes it
uneeded to call “ToObject(this)”, because it is guaranteed
that “this” is always a typed array of the corresponding
type.

After the previous change I am not completely sure that
creating a wrapper object for each element is always
needed. In my understanding, it is still needed, so I
have added the “TO_OBJECT_INLINE(element)” call inside
the loop. Note that I use the “SHOULD_CREATE_WRAPPER()”
macro from https://codereview.chromium.org/553413002/
so that CL has to be landed first.

[wingo, 2014-09-23 18:11:25]

Looking better, still a couple of comments.

https://codereview.chromium.org/583723002/diff/80001/src/harmony-typedarray.js
File src/harmony-typedarray.js (right):

https://codereview.chromium.org/583723002/diff/80001/src/harmony-typedarray.j...
src/harmony-typedarray.js:31: if (!(%_ClassOf(this) === 'NAME')) {
Not sure this is right; although the function isn't generic, as specified it
would seem to work on any kind of typedarray.

https://codereview.chromium.org/583723002/diff/80001/src/harmony-typedarray.j...
src/harmony-typedarray.js:34: }
Also what about the text that says "A TypeError exception is also immediately
thrown if the this value is detached or if its [[ViewedArrayBuffer]] is." ?

https://codereview.chromium.org/583723002/diff/80001/test/mjsunit/harmony/typ...
File test/mjsunit/harmony/typedarrays-foreach.js (right):

https://codereview.chromium.org/583723002/diff/80001/test/mjsunit/harmony/typ...
test/mjsunit/harmony/typedarrays-foreach.js:21: var a = new constructor(2);
tests needed that typed array foreach methods don't work on regular arrays

[aperez, 2014-09-30 14:17:15]

>
https://codereview.chromium.org/583723002/diff/80001/src/harmony-typedarray.j...
> src/harmony-typedarray.js:31: if (!(%_ClassOf(this) === 'NAME')) {
> Not sure this is right; although the function isn't generic, as specified it
> would seem to work on any kind of typedarray.

Good catch. According to the spec, the typed array methods belong to the
prototype of the internal “%TypedArray%” constructor, and all of the typed
arrays pick methods from there... so that means that yes, it should be
possible to transplant typed array methods to other typed arrays. (Not that
it makes a lot of sense, but well, that's what the spec says anyway :D)

I have added an “%IsTypedArray()” native, which is now used instead of
“%_ClassOf()”. Also, I have added code in the tests to check that the
method can be transplanted to other typed arrays (but not to other kinds
of objects).

>
https://codereview.chromium.org/583723002/diff/80001/src/harmony-typedarray.j...
> src/harmony-typedarray.js:34: }
> Also what about the text that says "A TypeError exception is also immediately
> thrown if the this value is detached or if its [[ViewedArrayBuffer]] is." ?

If I am reading the implementation correctly, the [[ViewedArrayBuffer]]
should never be detached after a typed array has been instantiated.

At first I tried implementing a “%TypedArrayBufferIsDetached()” native,
which check that the value returned by “JSTypedArray::buffer()” would
return a NULL value. Problem is: the buffer is lazily created when first
accessed, so the following would happen:

   a = new Int8Array(1);
   %TypedArrayBufferIsDetached(a);  // Returns true
   b = a.buffer;                    // Creates the buffer lazily if needed.
   a[0] = 1;                        // Creates the buffer lazily if needed.
   %TypedArrayBufferIsDetached(a);  // Now this returns false

So, checking that is unreliable, and anyway the buffer will be always
attached — or at least that's what the JS world sees.

>
https://codereview.chromium.org/583723002/diff/80001/test/mjsunit/harmony/typ...
> test/mjsunit/harmony/typedarrays-foreach.js:21: var a = new constructor(2);
> tests needed that typed array foreach methods don't work on regular arrays

Added.

[wingo, 2014-10-07 11:18:44]

https://codereview.chromium.org/583723002/diff/100001/src/harmony-typedarray.js
File src/harmony-typedarray.js (right):

https://codereview.chromium.org/583723002/diff/100001/src/harmony-typedarray....
src/harmony-typedarray.js:31: if (!%IsTypedArray(this)) {
Normally you don't want runtime calls in the hot path of a function, because
they can't be optimized without special support in the compiler.  Is there not
another way to do this?

https://codereview.chromium.org/583723002/diff/100001/src/harmony-typedarray....
src/harmony-typedarray.js:48: var needs_wrapper = SHOULD_CREATE_WRAPPER(f,
receiver);
this needs to be in an "else" case; i.e.

var needs_wrapper = false;
if (IS_NULL_OR_UNDEFINED(receiver)) {
  ...
} else {
  needs_wrapper = SHOULD_CREATE_WRAPPER(f, receiver);
}

[aperez, 2014-10-07 13:39:03]

On 2014/10/07 11:18:44, wingo wrote:
>
https://codereview.chromium.org/583723002/diff/100001/src/harmony-typedarray.js
> File src/harmony-typedarray.js (right):
> 
>
https://codereview.chromium.org/583723002/diff/100001/src/harmony-typedarray....
> src/harmony-typedarray.js:31: if (!%IsTypedArray(this)) {
> Normally you don't want runtime calls in the hot path of a function, because
> they can't be optimized without special support in the compiler.  Is there not
> another way to do this?

I am aware of this. For a while I thought about something like this:

  macro IS_TYPED_ARRAY_ITEM(ARRAY_ID, NAME, ELEMENT_SIZE)
    if (clsname == "NAME") return true;
  endmacro

  function IsTypedArray(obj) {
    var clsname = %_ClassOf(obj);
    TYPED_ARRAYS(IS_TYPED_ARRAY_ITEM)
    return false;
  }
  
but it looks a bit clumsy to me, and still involves using a runtime
function —thought “%_ClassOf()” gets inlined— and string comparisons.
Probably the best solution would be to make the “%IsTypedArray()”
inline-able.

WDYT?

>
https://codereview.chromium.org/583723002/diff/100001/src/harmony-typedarray....
> src/harmony-typedarray.js:48: var needs_wrapper = SHOULD_CREATE_WRAPPER(f,
> receiver);
> this needs to be in an "else" case; i.e.
> 
> var needs_wrapper = false;
> if (IS_NULL_OR_UNDEFINED(receiver)) {
>   ...
> } else {
>   needs_wrapper = SHOULD_CREATE_WRAPPER(f, receiver);
> }

Right. Fixed in the latest patch set.

[wingo, 2014-10-07 13:53:33]

wingo@igalia.com changed reviewers:
+ dslomov@chromium.org

[wingo, 2014-10-07 13:53:34]

Some comments below.  Seems strange to specify 9 different copies of the same
function, but if that's what the spec does, OK.  Adding Dmitry for review;
Dmitry what do you think about the IsTypedArray thing?

https://codereview.chromium.org/583723002/diff/120001/src/harmony-typedarray.js
File src/harmony-typedarray.js (right):

https://codereview.chromium.org/583723002/diff/120001/src/harmony-typedarray....
src/harmony-typedarray.js:32: throw MakeTypeError('not_typed_array', []);
So, what about doing this then

function IsTypedArray(obj) {
  switch (%_ClassOf(obj)) {
macro TYPED_ARRAY_CASE(ARRAY_ID, NAME, ELEMENT_SIZE)
    case "NAME":
endmacro
    TYPED_ARRAYS(TYPED_ARRAY_CASE)
      return true;
    default:
      return false;
}

....

and then in the foreach you do

if (%_ClassOf(obj) != "NAME" and !IsTypedArray(obj)) {
  throw foo
}

that way you have the small inline check for the most likely thing, and call out
otherwise for the general case.  Dunno, perhaps too much, but better than adding
a runtime function IMO.

Actually... at this point the function is completely the same for all of them. 
Seems silly to even generate them!  Is the spec's only requirement that they be
different functions?  I guess that helps monomorphize the call sites, still
seems a pity.

[aperez, 2014-10-07 14:28:42]

On 2014/10/07 13:53:34, wingo wrote:
> Some comments below.  Seems strange to specify 9 different copies of the same
> function, but if that's what the spec does, OK.  Adding Dmitry for review;
> Dmitry what do you think about the IsTypedArray thing?
> 
>
https://codereview.chromium.org/583723002/diff/120001/src/harmony-typedarray.js
> File src/harmony-typedarray.js (right):
> 
>
https://codereview.chromium.org/583723002/diff/120001/src/harmony-typedarray....
> src/harmony-typedarray.js:32: throw MakeTypeError('not_typed_array', []);
> So, what about doing this then
> 
> function IsTypedArray(obj) {
>   switch (%_ClassOf(obj)) {
> macro TYPED_ARRAY_CASE(ARRAY_ID, NAME, ELEMENT_SIZE)
>     case "NAME":
> endmacro
>     TYPED_ARRAYS(TYPED_ARRAY_CASE)
>       return true;
>     default:
>       return false;
> }
> 
> ....
> 
> and then in the foreach you do
> 
> if (%_ClassOf(obj) != "NAME" and !IsTypedArray(obj)) {
>   throw foo
> }
> 
> that way you have the small inline check for the most likely thing, and call
out
> otherwise for the general case.  Dunno, perhaps too much, but better than
adding
> a runtime function IMO.

Yes, this looks better than mine. And it looks much cleaner to use a
“switch” statement in “IsTypedArray()” than a chain of “if”s.

> Actually... at this point the function is completely the same for all of them.

> Seems silly to even generate them!  Is the spec's only requirement that they
be
> different functions?  I guess that helps monomorphize the call sites, still
> seems a pity.

The spec does not require them to be different functions, the idea of
having a copy for each typed array variant is monomophization, as you
have guessed.

Should I just change this to not create copied of the method? In this
case I would remove the “%_ClassOf()” call and do the check always
with “IsTypedArray()”.

[Dmitry Lomov (no reviews), 2014-10-08 12:14:48]

On 2014/10/07 13:53:34, wingo wrote:
> Some comments below.  Seems strange to specify 9 different copies of the same
> function, but if that's what the spec does, OK.  Adding Dmitry for review;
> Dmitry what do you think about the IsTypedArray thing?
> 
>
https://codereview.chromium.org/583723002/diff/120001/src/harmony-typedarray.js
> File src/harmony-typedarray.js (right):
> 
>
https://codereview.chromium.org/583723002/diff/120001/src/harmony-typedarray....
> src/harmony-typedarray.js:32: throw MakeTypeError('not_typed_array', []);
> So, what about doing this then
> 
> function IsTypedArray(obj) {
>   switch (%_ClassOf(obj)) {
> macro TYPED_ARRAY_CASE(ARRAY_ID, NAME, ELEMENT_SIZE)
>     case "NAME":
> endmacro
>     TYPED_ARRAYS(TYPED_ARRAY_CASE)
>       return true;
>     default:
>       return false;
> }
> 
> ....
> 
> and then in the foreach you do
> 
> if (%_ClassOf(obj) != "NAME" and !IsTypedArray(obj)) {
>   throw foo
> }
> 
> that way you have the small inline check for the most likely thing, and call
out
> otherwise for the general case.  Dunno, perhaps too much, but better than
adding
> a runtime function IMO.

I would actually prefer a runtime function. Easy to optimize if need be.
> 
> Actually... at this point the function is completely the same for all of them.

> Seems silly to even generate them!  Is the spec's only requirement that they
be
> different functions?  I guess that helps monomorphize the call sites, still
> seems a pity.

[wingo, 2014-10-08 13:37:58]

On 2014/10/08 12:14:48, Dmitry Lomov (chromium) wrote:
> I would actually prefer a runtime function. Easy to optimize if need be.

Cool. In that case lgtm as-is.  Dmitry, if you have time would you take a look? 
Thanks :)

[Dmitry Lomov (no reviews), 2014-10-08 13:57:11]

Some comments. Impl looks ok, tests need more work.

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

https://codereview.chromium.org/583723002/diff/120001/src/flag-definitions.h#...
src/flag-definitions.h:174: DEFINE_IMPLICATION(harmony, harmony_typedarrays)
Let's not have an extra flag for typed arrays, and put this under
harmony-arrays.

https://codereview.chromium.org/583723002/diff/120001/test/mjsunit/harmony/ty...
File test/mjsunit/harmony/typedarrays-foreach.js (right):

https://codereview.chromium.org/583723002/diff/120001/test/mjsunit/harmony/ty...
test/mjsunit/harmony/typedarrays-foreach.js:40: 
Add test for a function that throws exception midway.

Add a test that neuters a typed array midway (use %ArrayBufferNeuter).

Add a test with externalizes typed array midway (calls .buffer)
Add a test that constructs typed array using %TypedArray(ArrayBuffer)
constructor.

https://codereview.chromium.org/583723002/diff/120001/test/mjsunit/harmony/ty...
test/mjsunit/harmony/typedarrays-foreach.js:45: assertThrows(function () {
a.forEach(function (x) {}) }, TypeError);
Also test for non-array (e.g. {}). 

Use constructor.prototype.forEach.call.

Also test for primitives (1, "abc")

https://codereview.chromium.org/583723002/diff/120001/test/mjsunit/harmony/ty...
test/mjsunit/harmony/typedarrays-foreach.js:55: a.forEach =
Uint32Array.prototype.forEach;
Use Uint32Array.prototype.forEach.call - more straightforward.
Also, ensure that you execute all combinations (using typedArrayConstructors
array)

[aperez, 2014-10-14 12:40:10]

On 2014/10/08 13:57:11, Dmitry Lomov (chromium) wrote:
> Some comments. Impl looks ok, tests need more work.
> 
> https://codereview.chromium.org/583723002/diff/120001/src/flag-definitions.h
> File src/flag-definitions.h (right):
> 
>
https://codereview.chromium.org/583723002/diff/120001/src/flag-definitions.h#...
> src/flag-definitions.h:174: DEFINE_IMPLICATION(harmony, harmony_typedarrays)
> Let's not have an extra flag for typed arrays, and put this under
> harmony-arrays.

Done, this is now under the “harmony_arrays” flag. Also, the tests
are now updated with the suggested additions. PTAL :)

[Dmitry Lomov (no reviews), 2014-10-14 16:10:25]

Just a couple more nits

https://codereview.chromium.org/583723002/diff/140001/src/harmony-typedarray.js
File src/harmony-typedarray.js (right):

https://codereview.chromium.org/583723002/diff/140001/src/harmony-typedarray....
src/harmony-typedarray.js:27: macro TYPED_ARRAY_CONSTRUCTOR(ARRAY_ID, NAME,
ELEMENT_SIZE)
Change macro name

https://codereview.chromium.org/583723002/diff/140001/src/harmony-typedarray....
src/harmony-typedarray.js:49: needs_wrapper = SHOULD_CREATE_WRAPPER(f,
receiver);
Add a test that exercises wrapper creation and non-creation.

https://codereview.chromium.org/583723002/diff/140001/test/mjsunit/harmony/ty...
File test/mjsunit/harmony/typedarrays-foreach.js (right):

https://codereview.chromium.org/583723002/diff/140001/test/mjsunit/harmony/ty...
test/mjsunit/harmony/typedarrays-foreach.js:68: assertEquals(undefined, a[0]);
Assert how many times you have executed the function.
Assert that the array you see after neutering is actually neutered

[aperez, 2014-10-15 15:54:48]

Added more tests as requested.

https://codereview.chromium.org/583723002/diff/140001/src/harmony-typedarray.js
File src/harmony-typedarray.js (right):

https://codereview.chromium.org/583723002/diff/140001/src/harmony-typedarray....
src/harmony-typedarray.js:27: macro TYPED_ARRAY_CONSTRUCTOR(ARRAY_ID, NAME,
ELEMENT_SIZE)
On 2014/10/14 16:10:25, Dmitry Lomov (chromium) wrote:
> Change macro name

Acknowledged, changing to “TYPED_ARRAY_HARMONY_ADDITIONS”.

https://codereview.chromium.org/583723002/diff/140001/src/harmony-typedarray....
src/harmony-typedarray.js:49: needs_wrapper = SHOULD_CREATE_WRAPPER(f,
receiver);
On 2014/10/14 16:10:25, Dmitry Lomov (chromium) wrote:
> Add a test that exercises wrapper creation and non-creation. 

Acknowledged.

https://codereview.chromium.org/583723002/diff/140001/test/mjsunit/harmony/ty...
File test/mjsunit/harmony/typedarrays-foreach.js (right):

https://codereview.chromium.org/583723002/diff/140001/test/mjsunit/harmony/ty...
test/mjsunit/harmony/typedarrays-foreach.js:68: assertEquals(undefined, a[0]);
On 2014/10/14 16:10:25, Dmitry Lomov (chromium) wrote:
> Assert how many times you have executed the function.
> Assert that the array you see after neutering is actually neutered

Acknowledged.

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

lgtm, thanks!

[aperez, 2014-10-16 08:46:28]

On 2014/10/15 17:25:03, Dmitry Lomov (chromium) wrote:
> lgtm, thanks!

No prob. Could you please land this one for me? (maybe I can
just poke awingo for that :D)

[wingo, 2014-10-16 09:37:01]

On 2014/10/16 08:46:28, aperez wrote:
> On 2014/10/15 17:25:03, Dmitry Lomov (chromium) wrote:
> > lgtm, thanks!
> 
> No prob. Could you please land this one for me? (maybe I can
> just poke awingo for that :D)

I'll land

[wingo (chromium), 2014-10-16 10:58:26]

Committed patchset #9 manually as 24657 (presubmit successful).