Implement %TypedArray%.prototype.{map, filter}

RESOLVED FIXED in Firefox 38

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: 446240525, Assigned: tephra)

Tracking

({dev-doc-complete})

Trunk
mozilla38
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(2 attachments, 6 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
I have started work on this one.
Assignee: nobody → eric
Hey Eric! Do you have any questions?
(Assignee)

Comment 3

4 years ago
(In reply to Tom Schuster [:evilpie] from comment #2)
> Hey Eric! Do you have any questions?

Hey sorry I had some other work that got ahead of me. I'm still working on it, getting into it tonight so I will probably hit some issue and have a question later.
(Assignee)

Comment 4

4 years ago
Okay here's what I'm stuck at. Sorry if these seem trivial but I'm still learning the codebase.

Looking at https://people.mozilla.org/~jorendorff/es6-draft.html#sec-%typedarray%.prototype.map step 7 we should access [[TypedArrayName]] and I guess that is not implemented yet (there's no TypedArrayName function defined and just trying to do .name on the object don't seem to work). Any pointers on how to implement this?

I'm not so sure about the SpeciesConstructor and AllocateTypedArray abstract functions either. Any idea on how to do that part of the implementation?
These are very good questions. We indeed don't have [[TypedArrayName]] or SpeciesConstructor, because we don't implement proper sub-classing yet. So I suggest you just replace steps 7-11. with `new O.constructor(len)` for now. I will make sure to open a new bug so we don't forget to implement this.
(Assignee)

Comment 6

4 years ago
Thanks for the response. Should have a patch done for this weekend at the latest.
This enables getting original constructors for instances of builtins. It asserts if it's called for anything else. Given that TypedArray.prototype.map does a type check in step 2, and all other places in the spec that want us to retrieve original constructors (or prototypes) do, too, afaict, I don't see anything harmful about this approach.

Eric, this addresses your question about step 7. It can be implemented as `var defaultConstructor = _GetBuiltinConstructor(O)`.

As for step 8, ignore that for now. We don't support subclassing builtins yet, so it's not relevant.
Attachment #8559512 - Flags: review?(jwalden+bmo)
Assignee: eric → till
Status: NEW → ASSIGNED
Sorry, didn't mean to reassign the bug to me.

I also didn't see evilpie's comments. I don't think using O.constructor is a good idea. Also, with my attached patch, you can fully implement step 7, at least.
Assignee: till → eric
Posted file MozReview Request: bz://1121936/djc (obsolete) —
/r/3551 - Bug 1121936 - Part 1: add intrinsic for retrieving original builtin constructors. r=waldo
/r/3553 - Bug 1121935: Implement SpeciesConstructor stub for now
/r/3555 - Bug 1121935: Implement %TypedArray%.slice()
/r/3557 - Bug 1121935: Add tests for %TypedArray%.slice()

Pull down these commits:

hg pull review -r db8f3c33a61d6399cdf8a5a68fd7c20edcce7b50
Sorry, please ignore that review request.
Attachment #8561090 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
(In reply to Dirkjan Ochtman (:djc) from comment #9)
> Created attachment 8561090 [details]
> MozReview Request: bz://1121936/djc
> 
> /r/3551 - Bug 1121936 - Part 1: add intrinsic for retrieving original
> builtin constructors. r=waldo
> /r/3553 - Bug 1121935: Implement SpeciesConstructor stub for now
> /r/3555 - Bug 1121935: Implement %TypedArray%.slice()
> /r/3557 - Bug 1121935: Add tests for %TypedArray%.slice()
> 
> Pull down these commits:
> 
> hg pull review -r db8f3c33a61d6399cdf8a5a68fd7c20edcce7b50

Right will be looking into this tomorrow then.
(In reply to eric from comment #11)
> Right will be looking into this tomorrow then.

As I stated in comment 11, that request was erroneous; it has my changes for bug 1121935, but starts with the GetBuiltinConstructor patch that I stole from this bug, therefore MozReview appended comments to this issue. In any case, I don't think my patches should affect this bug.
(Assignee)

Comment 13

4 years ago
(In reply to Dirkjan Ochtman (:djc) from comment #12)
> (In reply to eric from comment #11)
> > Right will be looking into this tomorrow then.
> 
> As I stated in comment 11, that request was erroneous; it has my changes for
> bug 1121935, but starts with the GetBuiltinConstructor patch that I stole
> from this bug, therefore MozReview appended comments to this issue. In any
> case, I don't think my patches should affect this bug.

Ah sorry was a bit tired.
(Assignee)

Comment 14

4 years ago
Tests coming tomorrow.
Attachment #8562330 - Flags: review?(evilpies)
Comment on attachment 8562330 [details] [diff] [review]
Part 2: Implementation of map and filter.

Review of attachment 8562330 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/TypedArray.js
@@ +104,5 @@
> +    var O = this;
> +
> +    // Step 2-3.
> +    // This function is not generic.
> +    if (!IsObject(this) || !IsTypedArray(this)) {

nicer to use IsObject(O) here.

@@ +105,5 @@
> +
> +    // Step 2-3.
> +    // This function is not generic.
> +    if (!IsObject(this) || !IsTypedArray(this)) {
> +        return callFunction(CallTypedArrayMethodIfWrapped, this, callbackgn, thisArg,

Typo: callbackfn

@@ +121,5 @@
> +
> +    // Step 6.
> +    var T = thisArg;
> +
> +    // Step 7-9.

Step*s* please.

@@ +122,5 @@
> +    // Step 6.
> +    var T = thisArg;
> +
> +    // Step 7-9.
> +    // Step 8 ignored since SpeciesConstructor is not implemented.

I think it would be nice to at least have an almost useless implementation of SpeciesConstructor that we can extend.

@@ +127,5 @@
> +    var defaultConstructor = _GetBuiltinConstructor(O);
> +
> +    // Step 10.
> +    var kept = new Array();
> +

new List

@@ +133,5 @@
> +    var captured = 0;
> +
> +    // Step 11 and 13.
> +    for (var k = 0; k < len; k++) {
> +        // Step 13d and 13e.

13.d. and for everything else add the dot as well.

@@ +138,5 @@
> +        var selected = ToBoolean(callFunction(callbackfn, T, O[k], k, O));
> +        // Step 13f.
> +        if (selected) {
> +            // Step 13f.i.
> +            kept.push(O[k]);

You should introduce kValue, because you otherwise invoke O[k] twice. (However not observable anyway)

@@ +151,5 @@
> +    // Step 16.
> +    var n = 0;
> +
> +    // Step 17.
> +    for (var e of kept) {

I think after using `new List` you will have to use a plain old loop here.

@@ +153,5 @@
> +
> +    // Step 17.
> +    for (var e of kept) {
> +        // Step 17a-b.
> +        UnsafePutElements(A, n, kept[n]);

comment: I wonder if this is still safe after we introduce subclassing?

@@ +419,5 @@
>  }
>  
> +// ES6 draft rev32 (2015-02-02) 22.2.3.18 %TypedArray%.prototype.map(callbackfn [, thisArg]).
> +function TypedArrayMap(callbackfn, thisArg = undefined) {
> +    // Step 1.

Comments from filter apply here as well.

@@ +434,5 @@
> +    var len = TypedArrayLength(O);
> +
> +    // Step 5.
> +    if (arguments.length === 0)
> +        ThrowError(JSMSG_MISSING_FUN_ARG, 0, 'Array.prototype.map');

"%TypedArray%.prototype.map"
(In reply to Tom Schuster [:evilpie] from comment #15)
> @@ +122,5 @@
> > +    // Step 6.
> > +    var T = thisArg;
> > +
> > +    // Step 7-9.
> > +    // Step 8 ignored since SpeciesConstructor is not implemented.
> 
> I think it would be nice to at least have an almost useless implementation
> of SpeciesConstructor that we can extend.

You could reuse my patch for this:

https://bugzilla.mozilla.org/attachment.cgi?id=8561096

Cheers,

Dirkjan
Comment on attachment 8562330 [details] [diff] [review]
Part 2: Implementation of map and filter.

Quite solid already, but I would like to see a new version with those minor issues addressed. Thank you for making it this far.

Do you have any question about how to write a test for this change?
Attachment #8562330 - Flags: review?(evilpies)
Comment on attachment 8562330 [details] [diff] [review]
Part 2: Implementation of map and filter.

Review of attachment 8562330 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/TypedArray.js
@@ +121,5 @@
> +
> +    // Step 6.
> +    var T = thisArg;
> +
> +    // Step 7-9.

... and everywhere below when the comment mentions more than one step.

@@ +127,5 @@
> +    var defaultConstructor = _GetBuiltinConstructor(O);
> +
> +    // Step 10.
> +    var kept = new Array();
> +

This is important because otherwise content script can override the filter algorithms behavior by defining accessors on Array.prototype:
Object.defineProperty(Array.prototype, 0, {get: myEvilGetter, set: myEvilSetter});

Add a test that would've caught this.

@@ +151,5 @@
> +    // Step 16.
> +    var n = 0;
> +
> +    // Step 17.
> +    for (var e of kept) {

Additionally, using for .. of is a no-go in self-hosted code, because it relies on the iterator protocol, which can be influenced by changing, in this case, Array.prototype[Symbol.iterator]. Please also add a test that would've caught this.

@@ +153,5 @@
> +
> +    // Step 17.
> +    for (var e of kept) {
> +        // Step 17a-b.
> +        UnsafePutElements(A, n, kept[n]);

I'm pretty sure it is. By my reading 9.4.5[1] combined with the last paragraph of 22.2.4[2] taken together guarantee that TypedArray subclasses cannot override access behavior of indexed elements.

[1] people.mozilla.org/~jorendorff/es6-draft.html#sec-%typedarray%-intrinsic-object
[2] http://people.mozilla.org/~jorendorff/es6-draft.html#sec-typedarray-constructors

::: js/src/vm/TypedArrayObject.cpp
@@ +807,5 @@
>      JS_SELF_HOSTED_FN("findIndex", "TypedArrayFindIndex", 2, 0),
>      JS_SELF_HOSTED_FN("forEach", "TypedArrayForEach", 2, 0),
>      JS_SELF_HOSTED_FN("indexOf", "TypedArrayIndexOf", 2, 0),
>      JS_SELF_HOSTED_FN("join", "TypedArrayJoin", 1, 0),
> +    JS_SELF_HOSTED_FN("map", "TypedArrayMap", 2, 0),

Nit: move this down one line.
(Assignee)

Comment 19

4 years ago
(In reply to Tom Schuster [:evilpie] from comment #17)
> Comment on attachment 8562330 [details] [diff] [review]
> Part 2: Implementation of map and filter.
> 
> Quite solid already, but I would like to see a new version with those minor
> issues addressed. Thank you for making it this far.
> 
> Do you have any question about how to write a test for this change?

No just was to late to do it last night, something similar to the test for TypedArray.prototype.each would suffice?

Sorry for the typo must have missed that both in my own testing and my "make-sure-there-are-no-typos" final check last night...

(In reply to Tom Schuster [:evilpie] from comment #15)
> I think after using `new List` you will have to use a plain old loop here.

(In reply to Till Schneidereit [:till] from comment #18)
>Additionally, using for .. of is a no-go in self-hosted code, because it relies on the iterator protocol, which can be influenced by changing, in this case, Array.prototype[Symbol.iterator]. Please also add a test that would've caught this.

I had that at first but was unsure because of the wording of the spec here was different from usual, will be more quick to ask on IRC when something like that arises again thanks :)
> No just was to late to do it last night, something similar to the test for TypedArray.prototype.each
> would suffice?

It's a good start, but there are some more difficult things of course. First all of Tills suggestion. Then a test that would catch the bug that we would have introduced if we used kValue instead of O[k]. (The callback could change O[k], but that shouldn't matter).
SpeciesConstructor can throw.
I think I switched up kValue and O[k].
(In reply to eric from comment #19)
> I had that at first but was unsure because of the wording of the spec here
> was different from usual, will be more quick to ask on IRC when something
> like that arises again thanks :)

No worries at all - there's a huge amount of hidden complexity in all of this, so careful reviews are an absolute requirement.
(Assignee)

Comment 24

4 years ago
Had some issues with GIT but think I managed to fix it..
Attachment #8562330 - Attachment is obsolete: true
Attachment #8564725 - Flags: review?(evilpies)
(Assignee)

Comment 25

4 years ago
Posted patch bug1121936rev2.patch (obsolete) — Splinter Review
Submitted wrong version.
Attachment #8564725 - Attachment is obsolete: true
Attachment #8564725 - Flags: review?(evilpies)
Attachment #8564726 - Flags: review?(evilpies)
Comment on attachment 8564726 [details] [diff] [review]
bug1121936rev2.patch

Review of attachment 8564726 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/TypedArray.js
@@ +154,5 @@
> +
> +    // Steps 16 and 17.c.
> +    for (var n = 0; n < captured; n++) {
> +	// Steps 17.a-b.
> +	UnsafePutElements(A, n, kept[n]);

In the way SpeciesConstructor is implemented right now this is unsafe.
Consider:
var x = new Int32Array(5)
x.constructor = function() {}
x.map(v => v);

in that case new C, definitely doesn't result in an dense or typed array.

So, we should use A[n] = kept[n] here for now and open a new bug to correct this after we have subclassing.

@@ +420,5 @@
> +// ES6 draft rev32 (2015-02-02) 22.2.3.18 %TypedArray%.prototype.map(callbackfn [, thisArg]).
> +function TypedArrayMap(callbackfn, thisArg = undefined) {
> +    // Step 1.
> +    var O = this;
> +    

Nit: Remove whitespace.

@@ +433,5 @@
> +    var len = TypedArrayLength(O);
> +
> +    // Step 5.
> +    if (arguments.length === 0)
> +        ThrowError(JSMSG_MISSING_FUN_ARG, 0, 'TypedArray.prototype.map');

nit: "%TypedArray%.prototype.map"

@@ +449,5 @@
> +
> +    // Steps 10-11.
> +    var A = new C(len);
> +
> +    // Steps 12-13.

That doesn't seem necessary.

@@ +455,5 @@
> +    for (var k = 0; k < len; k++) {
> +        // Steps 13.d-e.
> +        var mappedValue = callFunction(callbackfn, T, O[k], k, O);
> +        // Steps 13.f-g.
> +        UnsafePutElements(A, k, mappedValue);

Same here A[k] = mappedValue.
Attachment #8564726 - Flags: review?(evilpies) → review+
(Assignee)

Comment 27

4 years ago
Will get right on fixing those.

(In reply to Tom Schuster [:evilpie] from comment #26)

> @@ +449,5 @@
> > +
> > +    // Steps 10-11.
> > +    var A = new C(len);
> > +
> > +    // Steps 12-13.
> 
> That doesn't seem necessary.
> 

Could you expand a bit, I don't think I understand why.
The next line already has "Steps 12, 13.a (implicit) and 13.h." and inside the loop you list all the other steps, so we avoid duplicating it like this. Just remove that comment.
(Assignee)

Comment 29

4 years ago
(In reply to Tom Schuster [:evilpie] from comment #28)
> The next line already has "Steps 12, 13.a (implicit) and 13.h." and inside
> the loop you list all the other steps, so we avoid duplicating it like this.
> Just remove that comment.

Ah sorry I thought you where referring to the `new C`. Thanks for clarifying.
Comment on attachment 8559512 [details] [diff] [review]
Part 1: add intrinsic for retrieving original builtin constructors

Review of attachment 8559512 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/SelfHosting.cpp
@@ +822,5 @@
> +    MOZ_ASSERT(args[0].isObject());
> +
> +    RootedObject object(cx, &args[0].toObject());
> +    JSProtoKey protoKey = StandardProtoKeyOrNull(object);
> +    MOZ_ASSERT(protoKey);

I'm very leery of adding a fully-generic method like this.  I would rather we had a method *only* for determining builtin typed array constructor.  If we need a method that works on some other kind of object, we should add exactly that narrow method.  A generic method risks people overusing it, without realizing the implicit restrictions on when it may validly be called.  Simply skimming all the hits for SpeciesConstructor and reasoning backward from that to a defensive non-generic class check or similar doesn't cut it for me.

So please rename this to something like ConstructorForTypedArray and add a MOZ_ASSERT(IsTypedArray(&args[0].toObject())) to the method body.
Attachment #8559512 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #30)
> I'm very leery of adding a fully-generic method like this.  I would rather
> we had a method *only* for determining builtin typed array constructor.  If
> we need a method that works on some other kind of object, we should add
> exactly that narrow method.  A generic method risks people overusing it,
> without realizing the implicit restrictions on when it may validly be
> called.

Ok, I changed the name and added the assert. I don't understand how exactly it could be overused as ISTM that the restrictions are enforced by the asserts, but we don't need to resolve this to get something landed here. Instead, we can get back to that discussion if and when we have a second use-case for such an intrinsic.

@eric, please use this new version and adapt your patches accordingly. We can land all of them together.
Attachment #8559512 - Attachment is obsolete: true
Attachment #8565915 - Flags: review+
Attachment #8565915 - Attachment is obsolete: true
I moved part 1 to bug 1121935, because otherwise we'd have a circular dependency between the two bugs. Please fix up the patch descriptions for part 2(-1) and part 3(-1) before landing.
Depends on: 1121935
https://treeherder.mozilla.org/logviewer.html#?job_id=5023860&repo=try
There is some weird test failure that I am not sure what is caused by.
(Assignee)

Comment 35

4 years ago
(In reply to Tom Schuster [:evilpie] from comment #34)
> https://treeherder.mozilla.org/logviewer.html#?job_id=5023860&repo=try
> There is some weird test failure that I am not sure what is caused by.

That is weird, If I run all the tests locally all pass.
It's actually quite incredible, I have an alter right before the reportCompare call that shows up.
Oh I found the problem. The test changes Array.prototype[@@iterator] and Array.prototype[0] and somehow this must screw up the harness, when reverting these changes after the test everything works fine.
Landed this! Thanks for all your effort, and I am sorry this was a little bit more difficult to get organized.
One small note: While I was fixing the test I noticed that some lines were using tabs.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f26a8bdf93e
(Assignee)

Comment 39

4 years ago
(In reply to Tom Schuster [:evilpie] from comment #38)
> Landed this! Thanks for all your effort, and I am sorry this was a little
> bit more difficult to get organized.
> One small note: While I was fixing the test I noticed that some lines were
> using tabs.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5f26a8bdf93e

Thanks!

Sorry should have gone noticed that, will make sure to be extra careful in the future.
https://hg.mozilla.org/mozilla-central/rev/5f26a8bdf93e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla38

Updated

4 years ago
Depends on: 1167700
You need to log in before you can comment on or make changes to this bug.