Implement Array[@@species] and ArraySpeciesCreate in Array prototype methods.

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: arai, Assigned: arai)

Tracking

(Blocks 1 bug, {dev-doc-complete})

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected, firefox48 fixed)

Details

()

Attachments

(18 attachments, 10 obsolete attachments)

111.91 KB, image/png
Details
1.04 KB, patch
efaust
: review+
Details | Diff | Splinter Review
2.30 KB, patch
efaust
: review+
Details | Diff | Splinter Review
6.21 KB, patch
efaust
: review+
Details | Diff | Splinter Review
11.28 KB, patch
efaust
: review+
Details | Diff | Splinter Review
6.36 KB, patch
efaust
: review+
Details | Diff | Splinter Review
67.27 KB, image/png
Details
3.23 KB, patch
efaust
: review+
Details | Diff | Splinter Review
4.35 KB, patch
efaust
: review+
Details | Diff | Splinter Review
5.48 KB, patch
efaust
: review+
Details | Diff | Splinter Review
18.33 KB, patch
jandem
: review+
Details | Diff | Splinter Review
6.62 KB, patch
jandem
: review+
Details | Diff | Splinter Review
1.89 KB, patch
efaust
: review+
Details | Diff | Splinter Review
855 bytes, patch
efaust
: review+
Details | Diff | Splinter Review
2.63 KB, patch
efaust
: review+
Details | Diff | Splinter Review
5.57 KB, patch
arai
: review+
Details | Diff | Splinter Review
839 bytes, patch
Details | Diff | Splinter Review
1.76 KB, patch
bholley
: review+
Details | Diff | Splinter Review
Assignee

Comment 1

4 years ago
fwiw, here's WIP patches
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9e5b09d44840

I think those shouldn't be landed before bug 1141863, since they introduce performance regression but has almost no merit without subclassing.
Duplicate of this bug: 911135
Assignee

Comment 3

4 years ago
Posted file array-species.zip (obsolete) —
Here's WIP patches.
Will post performance comparison in next comment.
Assignee

Comment 4

4 years ago
Posted image Performance comparison. (obsolete) —
Here's performance comparison with today's m-c.
Now Array#filter is self-hosted in m-c (bug 1193280), so this patch just makes it slow :P
Performance regression in map/filter (both self-hosted) in Ion-execution is significant.  inlining ArraySpeciesCreate self-hosted function may help.
Assignee

Comment 5

4 years ago
s/ArraySpeciesCreate self-hosted function/ArraySpeciesCreate function/
Assignee

Comment 6

4 years ago
Posted file WIP patches (obsolete) —
Rebased, and improved performance in map/filter
now ArraySpeciesCreate is implemented both with native and self-hosted.
Will post performance comparison in next comment.
Assignee: nobody → arai.unmht
Attachment #8681449 - Attachment is obsolete: true
Assignee

Comment 7

4 years ago
Attachment #8681450 - Attachment is obsolete: true
Assignee

Comment 8

4 years ago
Current patch is buggy with subclassing because of bug 1055472 comment #51.
Assignee

Comment 9

4 years ago
Prepared 12 patches

[Part 1]

For now, concat, slice, and splice are native function, so @@species getter is also native function.
Attachment #8687739 - Attachment is obsolete: true
Attachment #8696158 - Flags: review?(efaustbmo)
Assignee

Comment 10

4 years ago
[Part 2]

For performance reason, ArraySpeciesCreate is implemented both in native and selfhosted.

Native ArraySpeciesCreate is implemented with 2 functions
  * GetNonDefaultArraySpecies
    gets @@species value from given object's constructor.
    returns undefined if @@species value is Array, or really undefined,
    otherwise returns @@species value
    undefined means we can create an array without invoking the @@species value,
    and optimize more in caller

  * ArraySpeciesCreateWith
    construct an array (or something) with non-default @@species value
    it should be called only if GetNonDefaultArraySpecies returns non-undefined value

Added some helper functions
  * IsWrappedArrayConstructor
    checks if passed argument is an Array constructor from another global
    in most case this should return false

  * IsArraySpecies
    checks if passes argument's constructor's @@species is Array

  * GetNativeGetterPure
    it gets getter function without invoking it,
    and returns only if it's native function.
    so that we can check if it's origianl @@species getter.
Attachment #8696159 - Flags: review?(efaustbmo)
Assignee

Comment 11

4 years ago
[Part 3]

Self-hosted ArraySpeciesCreate does not call native ArraySpeciesCreate, to inline it in JIT.

IsArray, IsConstructor, and IsWrappedArrayConstructor are inlined in latter parts.
Attachment #8696160 - Flags: review?(efaustbmo)
Assignee

Comment 12

4 years ago
[Part 4]

IsArray can be replaced with Array.isArray, so it can be inlined with ArrayIsArray.
Attachment #8696161 - Flags: review?(efaustbmo)
Assignee

Comment 13

4 years ago
[Part 5]

IsConstructor can be inlined in almost same way as IsCallable, so the added code is similar to IsCallable's one
Attachment #8696162 - Flags: review?(efaustbmo)
Assignee

Comment 14

4 years ago
[Part 6]

IsWrappedArrayConstructor is replaced with a constant |false| when the argument is not a Proxy, so that it cannot be a wrapped thing.
If the passes argument is an Array comes from same global, this should always be inlined and return false.  Other cases are not inlined.
Attachment #8696163 - Flags: review?(efaustbmo)
Assignee

Comment 15

4 years ago
[Part 7]

Added ArraySpeciesCreate to Array.prototype.concat
  * ArrayConcatDense
    call array_concat_dense only if |this| value's constructor's @@species is Array

  * array_concat
    check @@species, and apply existing code only when @@species is Array
    otherwise create new Array with @@species value, and add each element with DefineElement
Attachment #8696164 - Flags: review?(efaustbmo)
Assignee

Comment 16

4 years ago
[Part 8]

Just changed ArrayFilter to call ArraySpeciesCreate instead of [].
Attachment #8696165 - Flags: review?(efaustbmo)
Assignee

Comment 17

4 years ago
[Part 9]

Just changed ArrayMap to call ArraySpeciesCreate instead of [], and update comments.
Attachment #8696166 - Flags: review?(efaustbmo)
Assignee

Comment 18

4 years ago
[Part 10]

Added ArraySpeciesCreate to Array.prototype.slice
  * ArraySliceOrdinary
    Split existing code for native Array from js::array_slice

  * js::array_slice
    call ArraySliceOrdinary only if @@species is Array
    otherwise create new Array with @@species value, and add each element with DefineElement

  * js::array_slice_dense
    use fast path only if |this| value's constructor's @@species is Array
Assignee

Comment 19

4 years ago
[Part 11]

Added ArraySpeciesCreate to Array.prototype.splice
  * ArraySpliceCopy
    Split copying code from js::array_splice_impl, that is used both in optimized (native Array) path and modified @@species path

  * js::array_splice_impl
    use existing path only if @@species is Array
    otherwise create new Array with @@species value

splice does not have *_dense variant for JIT
Attachment #8696168 - Flags: review?(efaustbmo)
Assignee

Comment 20

4 years ago
[Part 12]

Test for several @@species and constructor values, for each methods.
Attachment #8696169 - Flags: review?(efaustbmo)
Assignee

Updated

4 years ago
Attachment #8696167 - Flags: review?(efaustbmo)
Duplicate of this bug: 1233040
Comment on attachment 8696158 [details] [diff] [review]
Part 1: Implement Array[@@species] getter.

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

Yep.
Attachment #8696158 - Flags: review?(efaustbmo) → review+
Comment on attachment 8696159 [details] [diff] [review]
Part 2: Implement native ArraySpeciesCreate.

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

r=me with comments added and others addressed.

::: js/src/jsarray.cpp
@@ +884,5 @@
> +    // This must only return true if v is *the* Array constructor for the
> +    // current compartment; we rely on the fact that any other Array
> +    // constructor would be represented as a wrapper.
> +    return v.isObject() &&
> +           v.toObject().is<JSFunction>() &&

prefer |&& IsArrayConstructor(&v.toObject())|. That way we have one source of truth, and not duplicated entries.

@@ +904,5 @@
> +    if (!v.isObject()) {
> +        *result = false;
> +        return true;
> +    }
> +    if (v.toObject().is<CrossCompartmentWrapperObject>()) {

is<WrapperObject>(), I think should do.

@@ +937,5 @@
> +
> +    return getter == array_species_getter;
> +}
> +
> +/* ES6 9.4.2.3 steps 1-6. */

Unfortunately, the spec syntax changed, removing a step. Can you update the step numbers to match http://tc39.github.io/ecma262/#sec-arrayspeciescreate ?

@@ +953,5 @@
> +        pctor.setUndefined();
> +        return true;
> +    }
> +
> +    if (IsArraySpecies(cx, origArray)) {

This needs a comment, for sure. The idea is that if someone does something like:

{ constructor: Array }

that we can short-circuit, right?

@@ +989,5 @@
> +            pctor.setUndefined();
> +            return true;
> +        }
> +
> +        if (IsArrayConstructor(ctor)) {

Cute, to avoid calling it outright, but we need a comment here, also.

@@ +1021,5 @@
> +    ConstructArgs cargs(cx);
> +    if (!cargs.init(1))
> +        return false;
> +    if (length <= INT32_MAX)
> +        cargs[0].setInt32(length);

every last ounce of perf :P
Attachment #8696159 - Flags: review?(efaustbmo) → review+
Comment on attachment 8696160 [details] [diff] [review]
Part 3: Implement self-hosted ArraySpeciesCreate.

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

Cancelling review here, despite wanting to keep this more than part 2. Please rerequest review for both when we come up with something.

::: js/src/builtin/Array.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* ES6 9.4.2.3. */
> +function ArraySpeciesCreate(originalArray, length) {

We talked on IRC about why this is here for performance reasons, but I don't think we can maintain two copies of the algorithm.

Let's try keeping just the self-hosted one, and seeing if we can make that work via one method or another.
Attachment #8696160 - Flags: review?(efaustbmo)
Assignee

Comment 25

4 years ago
for now, tested 2 cases.

* self-host all methods
  just self-host slice/splice/concat

* call self-hosted ArraySpeciesCreate from native slice/splice/concat
  check IsArraySpecies (instead of GetNonDefaultArraySpecies in previous patch),
  and if it's false, call self-hosted ArraySpeciesCreate from native slice/splice/concat
  in most normal case, ArraySpeciesCreate won't be called (IsArraySpecies returns true for array from same global)

slice and concat could be self-hosted, as gradient in Ion execution is better than or almost equal to m-c and previous patch.  there is some offset (< 2ms), but it will be ignorable.

On the other hand, self-hosted splice is slow in some case with straight-forward implementation.  we might need optimized path (maybe wrapping current native impl) if we need self-hosting it.

Easiest solution will be self-host slice and concat, and keep splice native, with calling IsArraySpecies and self-hosted ArraySpeciesCreate there.
Do you think it's okay to keep IsArraySpecies?

(other option will be wrap splice with self-hosted function, as I said in IRC, if above solution is not acceptable, I'll try implementing it ;)
Flags: needinfo?(efaustbmo)
(In reply to Tooru Fujisawa [:arai] from comment #25)
> slice and concat could be self-hosted

FWIW, I've been thinking self-hosted Array#concat would be nice to have for Symbol.isConcatSpreadable as well..
Assignee

Updated

4 years ago
Depends on: 1233642
Assignee

Updated

4 years ago
Depends on: 1233643
(In reply to Tooru Fujisawa [:arai] from comment #25)
> Created attachment 8699450 [details]
> Performance comparison of slice/splice/concat.
> 
> for now, tested 2 cases.
> 
> * self-host all methods
>   just self-host slice/splice/concat
> 
> * call self-hosted ArraySpeciesCreate from native slice/splice/concat
>   check IsArraySpecies (instead of GetNonDefaultArraySpecies in previous
> patch),
>   and if it's false, call self-hosted ArraySpeciesCreate from native
> slice/splice/concat
>   in most normal case, ArraySpeciesCreate won't be called (IsArraySpecies
> returns true for array from same global)
> 
> slice and concat could be self-hosted, as gradient in Ion execution is
> better than or almost equal to m-c and previous patch.  there is some offset
> (< 2ms), but it will be ignorable.
> 
> On the other hand, self-hosted splice is slow in some case with
> straight-forward implementation.  we might need optimized path (maybe
> wrapping current native impl) if we need self-hosting it.
> 
> Easiest solution will be self-host slice and concat, and keep splice native,
> with calling IsArraySpecies and self-hosted ArraySpeciesCreate there.
> Do you think it's okay to keep IsArraySpecies?
> 
> (other option will be wrap splice with self-hosted function, as I said in
> IRC, if above solution is not acceptable, I'll try implementing it ;)

This sounds like a good plan to me, and if there is call to self-host the other two functions, then we do good along the way :)
Flags: needinfo?(efaustbmo)
Assignee

Comment 28

4 years ago
Thanks :)

Just found a bug for self-hosting splice (bug 890329)
See Also: → 890329
Assignee

Updated

4 years ago
Depends on: 1234880
Assignee

Comment 29

3 years ago
Comment on attachment 8696161 [details] [diff] [review]
Part 4: Inline IsArray intrinsic.

clearing r? for now.
Attachment #8696161 - Flags: review?(efaustbmo)
Assignee

Updated

3 years ago
Attachment #8696162 - Flags: review?(efaustbmo)
Assignee

Updated

3 years ago
Attachment #8696163 - Flags: review?(efaustbmo)
Assignee

Updated

3 years ago
Attachment #8696164 - Flags: review?(efaustbmo)
Assignee

Updated

3 years ago
Attachment #8696165 - Flags: review?(efaustbmo)
Assignee

Updated

3 years ago
Attachment #8696166 - Flags: review?(efaustbmo)
Assignee

Updated

3 years ago
Attachment #8696167 - Flags: review?(efaustbmo)
Assignee

Updated

3 years ago
Attachment #8696168 - Flags: review?(efaustbmo)
Assignee

Updated

3 years ago
Attachment #8696169 - Flags: review?(efaustbmo)
Assignee

Comment 30

3 years ago
here's current plan:
  1. self-hosted Array#concat in bug 1233642
  2. keep Array#slice and Array#splice native functions
  3. implement ArraySpeciesCreate only in self-hosted JS
  4. call self-hosted ArraySpeciesCreate from Array#slice and Array#splice
     only if IsArraySpecies is false
Assignee

Comment 31

3 years ago
here's WIP patch based on bug 1233642.
Assignee

Comment 32

3 years ago
Now @@species is called on self-hosted code, so moved it to self-hosted code.
Attachment #8696158 - Attachment is obsolete: true
Attachment #8728854 - Flags: review?(efaustbmo)
Assignee

Comment 33

3 years ago
slice and splice are still native functions,
IsArraySpecies is used to avoid calling self-hosted ArraySpeciesCreate function.

GetGetterPure will be added in bug 1165053
Attachment #8728856 - Flags: review?(efaustbmo)
Assignee

Comment 34

3 years ago
IsWrappedArrayConstructor is used to check ES 2016 spec 9.4.2.3 steps 5.b.1-iii.

https://tc39.github.io/ecma262/#sec-arrayspeciescreate
Attachment #8696163 - Attachment is obsolete: true
Attachment #8728858 - Flags: review?(efaustbmo)
Assignee

Comment 35

3 years ago
Almost same code as IsCallable.
Attachment #8696162 - Attachment is obsolete: true
Attachment #8728861 - Flags: review?(jdemooij)
Assignee

Comment 37

3 years ago
now ArraySpeciesCreate is implemented only in self-hosted code.
Attachment #8696159 - Attachment is obsolete: true
Attachment #8696160 - Attachment is obsolete: true
Attachment #8728880 - Flags: review?(efaustbmo)
Assignee

Comment 38

3 years ago
based on self-hosted A.p.concat in bug 1233642.
it's now so simple.
Attachment #8696161 - Attachment is obsolete: true
Attachment #8696164 - Attachment is obsolete: true
Attachment #8728881 - Flags: review?(efaustbmo)
Assignee

Comment 39

3 years ago
native ArraySpeciesCreate is just a wrapper to call self-hosted ArraySpeciesCreate.
Attachment #8728882 - Flags: review?(efaustbmo)
Attachment #8728861 - Flags: review?(jdemooij) → review+
Comment on attachment 8728862 [details] [diff] [review]
Part 5: Inline IsWrappedArrayConstructor intrinsic.

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

Nice

::: js/src/jit/MCallOptimize.cpp
@@ +2543,5 @@
>      return InliningStatus_Inlined;
>  }
>  
> +static inline bool
> +IsProxyClass(const Class* clasp)

We don't need this as there already is an IsProxyClass in vm/ProxyObject.h (it's used in this file so it should be available).

@@ +2566,5 @@
> +    TemporaryTypeSet* types = arg->resultTypeSet();
> +    switch (types->forAllClasses(constraints(), IsProxyClass)) {
> +      case TemporaryTypeSet::ForAllResult::ALL_FALSE: {
> +        break;
> +      }

Nit: can remove the {} here.
Attachment #8728862 - Flags: review?(jdemooij) → review+
Assignee

Updated

3 years ago
No longer depends on: 1233643
Attachment #8728854 - Flags: review?(efaustbmo) → review+
Comment on attachment 8728854 [details] [diff] [review]
Part 1: Implement Array[@@species] getter.

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

This is r+ either way, but I'm curious if we couldn't have one self-hosted function which is cloned into content compartments as the various @@species getters. Most of them are just "return this", right? I don't think it matters much, but it might be a nice simplification.
Comment on attachment 8728856 [details] [diff] [review]
Part 2: Add IsArraySpecies.

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

Looks good.

::: js/src/jsarray.cpp
@@ +872,5 @@
> +{
> +    // This must only return true if v is *the* Array constructor for the
> +    // current compartment; we rely on the fact that any other Array
> +    // constructor would be represented as a wrapper.
> +    return v.isObject() &&

OK, this isn't your fault, since you didn't write the patch, but I find it more intuitive to write this function:

static bool
IsArrayConstructor(const Value& v, JSContext* cx)
{
    return v.isObject() && v.toObject() == cx->global()->getConstructor(JSProto_Array).toObject();
}

What do you think?
Attachment #8728856 - Flags: review?(efaustbmo) → review+
Comment on attachment 8728858 [details] [diff] [review]
Part 3: Add IsWrappedArrayConstructor intrinsic.

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

wfm
Attachment #8728858 - Flags: review?(efaustbmo) → review+
Comment on attachment 8728880 [details] [diff] [review]
Part 6: Implement self-hosted ArraySpeciesCreate.

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

Ignore my ramblings about rewriting IsArrayConstructor from the earlier part. I see why it's how it is, now.

::: js/src/builtin/Array.js
@@ +889,5 @@
> +    // Step 4, 6.
> +    if (!IsArray(originalArray))
> +        return std_Array(length);
> +
> +    // Steps 3, 5.a.

I think calling this step 3 is just confusing the point. We can call this just 5.a

@@ +893,5 @@
> +    // Steps 3, 5.a.
> +    var C = originalArray.constructor;
> +
> +    // Step 5.b.
> +    if (IsConstructor(C) && IsWrappedArrayConstructor(C))

Now that I think about it, it's nice that we don't have to have a global involved in the IsArrayConstructor check, because it prevents any compartment concerns.

I guess we could do the same thing, except not take the cx, and do v.toObject().compartment()->global()->getConstructor(...) or something. Probably should just leave it ;)

@@ +902,5 @@
> +        // Step 6.c.i.
> +        C = C[std_species];
> +
> +        // Optimized path for an ordinary Array.
> +        if (C === GetBuiltinConstructor("Array"))

IsArrayConstructor(C) also works, right?

@@ +905,5 @@
> +        // Optimized path for an ordinary Array.
> +        if (C === GetBuiltinConstructor("Array"))
> +            return std_Array(length);
> +
> +        // Step 6.c.ii.

Also need the other step with the C is undefined check here. In the latest version that's step 6, but It's probably step 7 in the draft this is commented from.
Attachment #8728880 - Flags: review?(efaustbmo) → review+
Comment on attachment 8728882 [details] [diff] [review]
Part 7: Implement native ArraySpeciesCreate wrapper.

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

Thanks for unifying the two implementations.

::: js/src/jsarray.cpp
@@ +931,5 @@
> +    RootedId createId(cx, NameToId(cx->names().ArraySpeciesCreate));
> +    RootedFunction create(cx, JS::GetSelfHostedFunction(cx, "ArraySpeciesCreate", createId, 2));
> +
> +    InvokeArgs args(cx);
> +    if (!args.init(2))

Please use the (brand new) FixedInvokeArgs<2>

@@ +940,5 @@
> +    args[0].setObject(*origArray);
> +    if (length <= INT32_MAX)
> +        args[1].setInt32(length);
> +    else
> +        args[1].setNumber((double)length);

There's a NumberValue(uint32_t) overload that does exactly this. Please use it here.
Attachment #8728882 - Flags: review?(efaustbmo) → review+
Comment on attachment 8728881 [details] [diff] [review]
Part 8: Use ArraySpeciesCreate in Array.prototype.concat.

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

::: js/src/builtin/Array.js
@@ +929,5 @@
>      // Step 1.
>      var O = ToObject(this);
>  
>      // Step 2.
> +    var A = ArraySpeciesCreate(O, 0);

blech. I hate that O and 0 look so similar. No change needed, just grumbling. If you wanted to make it |obj|, I wouldn't object, but I understand why the spec names were used.
Attachment #8728881 - Flags: review?(efaustbmo) → review+
Assignee

Updated

3 years ago
Attachment #8696165 - Flags: review?(efaustbmo)
Assignee

Updated

3 years ago
Attachment #8696166 - Flags: review?(efaustbmo)
Assignee

Updated

3 years ago
Attachment #8696167 - Flags: review?(efaustbmo)
Assignee

Updated

3 years ago
Attachment #8696168 - Flags: review?(efaustbmo)
Assignee

Updated

3 years ago
Attachment #8696169 - Flags: review?(efaustbmo)
Assignee

Comment 47

3 years ago
Thank you for reviewing :D

(In reply to Eric Faust [:efaust] from comment #44)
> Comment on attachment 8728880 [details] [diff] [review]
> Part 6: Implement self-hosted ArraySpeciesCreate.
> @@ +902,5 @@
> > +        // Step 6.c.i.
> > +        C = C[std_species];
> > +
> > +        // Optimized path for an ordinary Array.
> > +        if (C === GetBuiltinConstructor("Array"))
> 
> IsArrayConstructor(C) also works, right?

yeah, but comparison with GetBuiltinConstructor can be done in JIT code, without adding self-hosting instrinsic and inlining for IsArrayConstructor.


> @@ +905,5 @@
> > +        // Optimized path for an ordinary Array.
> > +        if (C === GetBuiltinConstructor("Array"))
> > +            return std_Array(length);
> > +
> > +        // Step 6.c.ii.
> 
> Also need the other step with the C is undefined check here. In the latest
> version that's step 6, but It's probably step 7 in the draft this is
> commented from.

Sorry, those are Steps 5.xxx, and Step 6 (If C is undefined, return ? ArrayCreate(length)) is just below the block

>     // Step 6.
>     if (C === undefined)
>         return std_Array(length);
Attachment #8696165 - Flags: review?(efaustbmo) → review+
Comment on attachment 8696166 [details] [diff] [review]
Part 9: Use ArraySpeciesCreate in Array.prototype.map.

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

Thanks for also updating comments!
Attachment #8696166 - Flags: review?(efaustbmo) → review+
Comment on attachment 8696167 [details] [diff] [review]
Part 10: Use ArraySpeciesCreate in Array.prototype.slice.

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

wfm
Attachment #8696167 - Flags: review?(efaustbmo) → review+
Comment on attachment 8696168 [details] [diff] [review]
Part 11: Use ArraySpeciesCreate in Array.prototype.splice.

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

Strong. Nice refactor.
Attachment #8696168 - Flags: review?(efaustbmo) → review+
Comment on attachment 8696169 [details] [diff] [review]
Part 12: Add tests for ArraySpeciesCreate.

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

This is remarkably thorough. Thank you for your attention to detail.

::: js/src/tests/ecma_6/Array/species.js
@@ +45,5 @@
> +
> +      return undefined;
> +    }
> +  });
> +  a = new Proxy([1, 2, 3, 4, 5], {

Thanks for testing the proxy case!

@@ +77,5 @@
> +  a.constructor = {
> +    [Symbol.species]: null
> +  };
> +  b = a[funcName](...args);
> +  assertEq(b.constructor, Array);

Nice.

@@ +115,5 @@
> +  });
> +  b = a[funcName](...args);
> +  assertEq(b.constructor, Array);
> +
> +  // @@species from different global

:)

@@ +149,5 @@
> +  b = g.a[funcName](...args);
> +  assertEq(b.constructor, g.Array);
> +
> +  // subclasses
> +  if (classesEnabled()) {

don't need classesEnabled() anymore :)
Attachment #8696169 - Flags: review?(efaustbmo) → review+
Assignee

Comment 52

3 years ago
maybe related to bug 892903, basic/splice-check-steps.js now logs "get-constructor," for ArraySpeciesCreate.
just added that to expected_order for each test, that invokes ArraySpeciesCreate.
Attachment #8734609 - Flags: review?(evilpies)
Assignee

Comment 53

3 years ago
Comment on attachment 8734609 [details] [diff] [review]
Part 12.1: Update basic/splice-check-steps.js to follow @@species.

r=evilpie on IRC
Attachment #8734609 - Flags: review?(evilpies) → review+
Assignee

Comment 54

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8825d25eab2d107c5be515b15ef4b5ee1de79421
Bug 1165052 - Part 1: Implement Array[@@species] getter. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a3147dc1003ff95a0b4b79d924113c1466baec9
Bug 1165052 - Part 2: Add IsArraySpecies. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e02cbf51e4540dec3716e5ec9b415b632e8473c
Bug 1165052 - Part 3: Add IsWrappedArrayConstructor intrinsic. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdbb1c215d783a85c27d71ed8bb832690962c948
Bug 1165052 - Part 4: Inline IsConstructor intrinsic. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb171b15a882723b189e2fefa2a06f27e8f907c5
Bug 1165052 - Part 5: Inline IsWrappedArrayConstructor intrinsic. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/0344adae92f3c3fe026a7c365689343cde287fe1
Bug 1165052 - Part 6: Implement self-hosted ArraySpeciesCreate. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa59399ae8e09a153b63210b966c6364376c1e0c
Bug 1165052 - Part 7: Implement native ArraySpeciesCreate wrapper. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c725d3463f112ca08e0c0a05537a6077a915723
Bug 1165052 - Part 8: Use ArraySpeciesCreate in Array.prototype.concat. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/e49d0dfcaa5c7415c94b830e11dda40700522011
Bug 1165052 - Part 9: Use ArraySpeciesCreate in Array.prototype.filter. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d13efb13fec2a7843d65c27c9862bd5e51c457d
Bug 1165052 - Part 10: Use ArraySpeciesCreate in Array.prototype.map. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/e682faee5bff7c99ed7dc045f67e5324525d396e
Bug 1165052 - Part 11: Use ArraySpeciesCreate in Array.prototype.slice. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e108d752209e5dc17c91775974d0b82b7ff2f93
Bug 1165052 - Part 12: Use ArraySpeciesCreate in Array.prototype.splice. r=efaust,evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/52a142a57bf629018a0b16e85d75450e1699c0da
Bug 1165052 - Part 13: Add tests for ArraySpeciesCreate. r=efaust
Assignee

Comment 55

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a74723c089e962e2f3f6c5aee205b55b8f17e00
Backed out changeset 52a142a57bf6 (bug 1165052)

https://hg.mozilla.org/integration/mozilla-inbound/rev/696987b80ad49e0e1225f6ffcb1e59cb991f4f51
Backed out changeset 5e108d752209 (bug 1165052)

https://hg.mozilla.org/integration/mozilla-inbound/rev/b38df48d4638a80a73e3d4dc6d935f1bef122ff7
Backed out changeset e682faee5bff (bug 1165052)

https://hg.mozilla.org/integration/mozilla-inbound/rev/d61e32f1ad9cf2c377317066bb668f7e7bb86e2e
Backed out changeset 5d13efb13fec (bug 1165052)

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e3a42336a3738b82d35e5fb09d9aef347106ba9
Backed out changeset e49d0dfcaa5c (bug 1165052)

https://hg.mozilla.org/integration/mozilla-inbound/rev/64788d3e93a9b95a3111faf76af4a0f7b690d1d6
Backed out changeset 0c725d3463f1 (bug 1165052)

https://hg.mozilla.org/integration/mozilla-inbound/rev/eab50485823b191b712479eb887c00ed21a0e5a5
Backed out changeset aa59399ae8e0 (bug 1165052)

https://hg.mozilla.org/integration/mozilla-inbound/rev/70de0d01ec44e7f40ff784bf3aab9fdc5e49cfd8
Backed out changeset 0344adae92f3 (bug 1165052)

https://hg.mozilla.org/integration/mozilla-inbound/rev/24ce44d6133013f942fa9c1bf5b641d468da0efc
Backed out changeset cb171b15a882 (bug 1165052)

https://hg.mozilla.org/integration/mozilla-inbound/rev/23e771ef64a10cb55b7d938b9d5dd60444c15013
Backed out changeset fdbb1c215d78 (bug 1165052)

https://hg.mozilla.org/integration/mozilla-inbound/rev/a08059a8e1785f9d18b776a09e98afcc24a7ad13
Backed out changeset 5e02cbf51e45 (bug 1165052)

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd9eee8108af36b5f26dce4c1934476cc3cd38d5
Backed out changeset 2a3147dc1003 (bug 1165052)

https://hg.mozilla.org/integration/mozilla-inbound/rev/705cf9ac47e9ccb246d18b5c8a0b6ce6419fa556
Backed out changeset 8825d25eab2d (bug 1165052)
Assignee

Comment 56

3 years ago
https://dxr.mozilla.org/mozilla-central/source/dom/bindings/test/test_oom_reporting.html

this test expects Array#splice to execute NewFullyAllocatedArrayTryReuseGroup or some fully allocated variants, to report OOM instantly, but now we use std_Array, and it doesn't fully allocate at the object allocation, and it takes too long time until hit actual OOM while defining properties.

Do you know any other way to emulate this test with other function?
or can we just disable this test?
Attachment #8736216 - Flags: review?(efaustbmo)
Assignee

Comment 57

3 years ago
Related to bug 887016, Array just works by adding Symbol.species to the list.
(not like RegExp, it doesn't work by adding Symbol.species, and also not like TypedArray, that doesn't fail even I don't add Symbol.species)
Attachment #8736219 - Flags: review?(bobbyholley)
Attachment #8736219 - Flags: review?(bobbyholley) → review+
Assignee

Comment 58

3 years ago
Comment on attachment 8736216 [details] [diff] [review]
Part 12.1: Disable test_oom_reporting.html because the test is no more valid.

Discussed on IRC.
I'll add a new testing function that throws OOM, and use it in test_oom_reporting.html, instead of Array#splice.
Attachment #8736216 - Flags: review?(efaustbmo)
Assignee

Comment 59

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac7bd86f3e6020d1afd3271bb26e04e146472b29
Bug 1165052 - Part 0: Add throwOutOfMemory testing function and use it instead of Array.prototype.splice in test_oom_reporting.html. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/35b9afd414ed452fdfee91b89a6dc51de9e3868c
Bug 1165052 - Part 1: Implement Array[@@species] getter. r=efaust,bholley

https://hg.mozilla.org/integration/mozilla-inbound/rev/f15991a13732a4d966fad452bfc8f769fb258859
Bug 1165052 - Part 2: Add IsArraySpecies. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/ae46f7e09a221fa5006e4abd2ba458998328d5c4
Bug 1165052 - Part 3: Add IsWrappedArrayConstructor intrinsic. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecb8f8821de2ab318eb4a0d863b5d9c1cd97d70f
Bug 1165052 - Part 4: Inline IsConstructor intrinsic. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/62974e03b350265b4413963e34ccef88bdb9d3d4
Bug 1165052 - Part 5: Inline IsWrappedArrayConstructor intrinsic. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/a06b6d9f65402fb736f55ab5b378dd3759adf9f7
Bug 1165052 - Part 6: Implement self-hosted ArraySpeciesCreate. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/450ea0e5c8cb17e38ac80c456b8674bfa4ade377
Bug 1165052 - Part 7: Implement native ArraySpeciesCreate wrapper. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2ae0fa9376081d2b7c509f213458bb850ae27d8
Bug 1165052 - Part 8: Use ArraySpeciesCreate in Array.prototype.concat. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc13616aed735278e640805522ec4866f16305de
Bug 1165052 - Part 9: Use ArraySpeciesCreate in Array.prototype.filter. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7d9523b227c6c30f8a894bacdeeefb6b05d25aa
Bug 1165052 - Part 10: Use ArraySpeciesCreate in Array.prototype.map. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/830dc8e6e370b2a41b2a175294334ca60543c8bd
Bug 1165052 - Part 11: Use ArraySpeciesCreate in Array.prototype.slice. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/85fc8ee6d839e85c5f7a5880b94294befce00be8
Bug 1165052 - Part 12: Use ArraySpeciesCreate in Array.prototype.splice. r=efaust,evilpie

https://hg.mozilla.org/integration/mozilla-inbound/rev/28226109634e0589f8ce078ee4985033bc64b08a
Bug 1165052 - Part 13: Add tests for ArraySpeciesCreate. r=efaust
Depends on: 1262967
Assignee

Updated

3 years ago
Depends on: 1263525
Great, thank you for the doc updates!
Depends on: 1263888
Assignee

Updated

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