TypedArray.prototype.every method

TypedArray.prototype.every method

BUG=v8:3578
LOG=Y
R=adamk@chromium.org, dslomov@chromium.org, svenpanne@chromium.org

Committed: https://crrev.com/60e674c11efd62a8bdd086916e489476484eedd8
Cr-Commit-Position: refs/heads/master@{#28301}

[dehrenberg, 2015-05-06 22:04:55]

Something funny happened in rebasing this patch before mailing, and it doesn't
seem to pass the test in this form. I'm trying to figure it out.

[adamk, 2015-05-06 23:29:04]

I have a high-level question: it looks like harmony-typedarray.js currently
creates many versions of the "of" and "forEach" methods, and then installs each
one on each TypedArray prototype (once on each of Uint. This doesn't match my
reading of the spec, which has a %TypedArray% constructor and a
%TypedArrayPrototype% that acts as its prototype object. All the typed arrays
inherit from these. And all of the new methods are supposed to be on
%TypedArrayPrototype% (and thus there should be a single function per realm).

I'm new to the TypedArray world, and so I suspect not only Dmitry but also Dan
know more at this point. What am I missing?

[dehrenberg, 2015-05-07 00:32:13]

On 2015/05/06 23:29:04, adamk wrote:
> I have a high-level question: it looks like harmony-typedarray.js currently
> creates many versions of the "of" and "forEach" methods, and then installs
each
> one on each TypedArray prototype (once on each of Uint. This doesn't match my
> reading of the spec, which has a %TypedArray% constructor and a
> %TypedArrayPrototype% that acts as its prototype object. All the typed arrays
> inherit from these. And all of the new methods are supposed to be on
> %TypedArrayPrototype% (and thus there should be a single function per realm).
> 
> I'm new to the TypedArray world, and so I suspect not only Dmitry but also Dan
> know more at this point. What am I missing?

Oh, thanks for pointing that out; I was assuming that %TypedArray% was standing
in for each of the TypedArray subclasses, but the text is actually pretty clear
that it's a superclass which isn't exposed through a particular name. Oops. The
distinction must be observable through introspection.

But despite that, I am wondering if it still makes sense to clone the method for
each of the subclasses. Won't it become polymorphic if it's just on the
superclass, but it'll stay monomorphic if it's defined for each subclass? The
current implementation of forEach, for example, seems to follow the spec just
fine in that you could do something silly like Uint8Array.prototype.forEach =
new Uint32Array([]).forEach and it would work (element size is not hard-coded,
for example). Macros in that file define the ELEMENT_SIZE but then it's never
used.

[adamk, 2015-05-07 00:41:55]

On 2015/05/07 00:32:13, dehrenberg wrote:
> On 2015/05/06 23:29:04, adamk wrote:
> > I have a high-level question: it looks like harmony-typedarray.js currently
> > creates many versions of the "of" and "forEach" methods, and then installs
> each
> > one on each TypedArray prototype (once on each of Uint. This doesn't match
my
> > reading of the spec, which has a %TypedArray% constructor and a
> > %TypedArrayPrototype% that acts as its prototype object. All the typed
arrays
> > inherit from these. And all of the new methods are supposed to be on
> > %TypedArrayPrototype% (and thus there should be a single function per
realm).
> > 
> > I'm new to the TypedArray world, and so I suspect not only Dmitry but also
Dan
> > know more at this point. What am I missing?
> 
> Oh, thanks for pointing that out; I was assuming that %TypedArray% was
standing
> in for each of the TypedArray subclasses, but the text is actually pretty
clear
> that it's a superclass which isn't exposed through a particular name. Oops.
The
> distinction must be observable through introspection.
> 
> But despite that, I am wondering if it still makes sense to clone the method
for
> each of the subclasses. Won't it become polymorphic if it's just on the
> superclass, but it'll stay monomorphic if it's defined for each subclass? The
> current implementation of forEach, for example, seems to follow the spec just
> fine in that you could do something silly like Uint8Array.prototype.forEach =
> new Uint32Array([]).forEach and it would work (element size is not hard-coded,
> for example). Macros in that file define the ELEMENT_SIZE but then it's never
> used.

Cloning the methods is simply not spec-compliant, as expressions like

Int8Array.prototype.forEach === Int16Array.prototype.forEach

should return true. I agree that there may be performance implications to this,
but that's a separate concern.

Besides the spec-compliance, there's also been significant work lately to reduce
the size of the native context snapshot, and avoiding duplicating these
functions at least limits that growth a little bit.

[caitp (gmail), 2015-05-07 01:02:00]

What I would suggest doing, is to implement these methods as generic (working
for each TypedArray, matching the spec as closely as possible), and just install
the same method on each TypedArray class.

Then it's trivial to move them onto the %TypedArray% intrinsic base class once
it exists.

[dehrenberg, 2015-05-07 02:19:44]

FWIW v3 actually works. Yay tests. I guess I'll work on getting this proto chain
and set of methods set up properly first before trying to get this submitted
though. But really it's sorta trivial no matter what order things get done in,
it just all needs to get done.

Or were you suggesting that as a performance thing? I don't see how it'd help
performance (unless it was different copies of the same method body, like the
current code does). If it's the same actual function identity, won't it become
one big piece of polymorphic code? (Or, if otherwise, anyone have pointers to
understand how that's implemented?)

[caitp (gmail), 2015-05-07 02:22:26]

On 2015/05/07 02:19:44, dehrenberg wrote:
> FWIW v3 actually works. Yay tests. I guess I'll work on getting this proto
chain
> and set of methods set up properly first before trying to get this submitted
> though. But really it's sorta trivial no matter what order things get done in,
> it just all needs to get done.
> 
> Or were you suggesting that as a performance thing? I don't see how it'd help
> performance (unless it was different copies of the same method body, like the
> current code does). If it's the same actual function identity, won't it become
> one big piece of polymorphic code? (Or, if otherwise, anyone have pointers to
> understand how that's implemented?)

Not a performance thing. Just, getting it as close to the spec as possible,
without worrying about the extra prototype. It's a good compromise until the
base %TypedArray% class has landed

[dehrenberg, 2015-05-07 02:25:44]

dehrenberg@chromium.org changed reviewers:
+ caitpotter88@gmail.com

[arv (Not doing code reviews), 2015-05-07 13:00:10]

arv@chromium.org changed reviewers:
+ arv@chromium.org

[arv (Not doing code reviews), 2015-05-07 13:00:11]

LGTM

https://codereview.chromium.org/1128273002/diff/40001/test/mjsunit/harmony/ty...
File test/mjsunit/harmony/typedarrays-every.js (right):

https://codereview.chromium.org/1128273002/diff/40001/test/mjsunit/harmony/ty...
test/mjsunit/harmony/typedarrays-every.js:33: assertEquals(false, result);
there is also assertFalse.

https://codereview.chromium.org/1128273002/diff/40001/test/mjsunit/harmony/ty...
test/mjsunit/harmony/typedarrays-every.js:43: result = a.every(function (n,
index, array) { return n == index && n < this.value; }, o);
long line

[dehrenberg, 2015-05-07 14:14:47]

The CQ bit was checked by dehrenberg@chromium.org

[dehrenberg, 2015-05-07 14:14:47]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org
Link to the patchset: https://codereview.chromium.org/1128273002/#ps60001
(title: "Style fixes")

[dehrenberg, 2015-05-07 14:14:57]

The CQ bit was unchecked by dehrenberg@chromium.org

[commit-bot: I haz the power, 2015-05-07 14:14:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1128273002/60001

[dehrenberg, 2015-05-07 14:15:09]

The CQ bit was checked by dehrenberg@chromium.org

[commit-bot: I haz the power, 2015-05-07 14:15:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1128273002/60001

[commit-bot: I haz the power, 2015-05-07 14:25:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-07 14:25:48]

Try jobs failed on following builders:
  v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/194)

[dehrenberg, 2015-05-07 15:06:37]

The CQ bit was checked by dehrenberg@chromium.org

[dehrenberg, 2015-05-07 15:06:38]

The patchset sent to the CQ was uploaded after l-g-t-m from arv@chromium.org
Link to the patchset: https://codereview.chromium.org/1128273002/#ps80001
(title: "s/ToObject/$toObject/")

[commit-bot: I haz the power, 2015-05-07 15:06:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1128273002/80001

[commit-bot: I haz the power, 2015-05-07 15:54:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-07 15:54:36]

Patchset 5 (id:??) landed as
https://crrev.com/60e674c11efd62a8bdd086916e489476484eedd8
Cr-Commit-Position: refs/heads/master@{#28301}