Closed
Bug 1165052
Opened 10 years ago
Closed 9 years ago
Implement Array[@@species] and ArraySpeciesCreate in Array prototype methods.
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: arai, Assigned: arai)
References
(Blocks 1 open bug, )
Details
(Keywords: dev-doc-complete)
Attachments
(18 files, 10 obsolete files)
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 |
derived from bug 1131043.
Array[@@species] is used by ArraySpeciesCreate, which is used in following methods. we should add them at the same time, for feature test.
Array.prototype.concat
Array.prototype.filter
Array.prototype.map
Array.prototype.slice
Array.prototype.splice
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-get-array-@@species
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-arrayspeciescreate
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.concat
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.filter
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.map
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.slice
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.splice
Assignee | ||
Comment 1•9 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.
Assignee | ||
Comment 3•9 years ago
|
||
Here's WIP patches.
Will post performance comparison in next comment.
Assignee | ||
Comment 4•9 years ago
|
||
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•9 years ago
|
||
s/ArraySpeciesCreate self-hosted function/ArraySpeciesCreate function/
Assignee | ||
Comment 6•9 years ago
|
||
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•9 years ago
|
||
Attachment #8681450 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
Current patch is buggy with subclassing because of bug 1055472 comment #51.
Assignee | ||
Comment 9•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 years ago
|
||
[Part 8]
Just changed ArrayFilter to call ArraySpeciesCreate instead of [].
Attachment #8696165 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 17•9 years ago
|
||
[Part 9]
Just changed ArrayMap to call ArraySpeciesCreate instead of [], and update comments.
Attachment #8696166 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 18•9 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•9 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•9 years ago
|
||
[Part 12]
Test for several @@species and constructor values, for each methods.
Attachment #8696169 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8696167 -
Flags: review?(efaustbmo)
Comment 22•9 years ago
|
||
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 23•9 years ago
|
||
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 24•9 years ago
|
||
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•9 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)
Comment 26•9 years ago
|
||
(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..
Comment 27•9 years ago
|
||
(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•9 years ago
|
||
Thanks :)
Just found a bug for self-hosting splice (bug 890329)
See Also: → 890329
Assignee | ||
Comment 29•9 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•9 years ago
|
Attachment #8696162 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8696163 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8696164 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8696165 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8696166 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8696167 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8696168 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8696169 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 30•9 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•9 years ago
|
||
here's WIP patch based on bug 1233642.
Assignee | ||
Comment 32•9 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•9 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•9 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•9 years ago
|
||
Almost same code as IsCallable.
Attachment #8696162 -
Attachment is obsolete: true
Attachment #8728861 -
Flags: review?(jdemooij)
Assignee | ||
Comment 36•9 years ago
|
||
Attachment #8728862 -
Flags: review?(jdemooij)
Assignee | ||
Comment 37•9 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•9 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•9 years ago
|
||
native ArraySpeciesCreate is just a wrapper to call self-hosted ArraySpeciesCreate.
Attachment #8728882 -
Flags: review?(efaustbmo)
Updated•9 years ago
|
Attachment #8728861 -
Flags: review?(jdemooij) → review+
Comment 40•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8728854 -
Flags: review?(efaustbmo) → review+
Comment 41•9 years ago
|
||
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 42•9 years ago
|
||
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 43•9 years ago
|
||
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 44•9 years ago
|
||
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 45•9 years ago
|
||
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 46•9 years ago
|
||
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•9 years ago
|
Attachment #8696165 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8696166 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8696167 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8696168 -
Flags: review?(efaustbmo)
Assignee | ||
Updated•9 years ago
|
Attachment #8696169 -
Flags: review?(efaustbmo)
Assignee | ||
Comment 47•9 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);
Updated•9 years ago
|
Attachment #8696165 -
Flags: review?(efaustbmo) → review+
Comment 48•9 years ago
|
||
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 49•9 years ago
|
||
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 50•9 years ago
|
||
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 51•9 years ago
|
||
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•9 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•9 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•9 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•9 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•9 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•9 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)
Updated•9 years ago
|
Attachment #8736219 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 58•9 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•9 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
Comment 60•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ac7bd86f3e60
https://hg.mozilla.org/mozilla-central/rev/35b9afd414ed
https://hg.mozilla.org/mozilla-central/rev/f15991a13732
https://hg.mozilla.org/mozilla-central/rev/ae46f7e09a22
https://hg.mozilla.org/mozilla-central/rev/ecb8f8821de2
https://hg.mozilla.org/mozilla-central/rev/62974e03b350
https://hg.mozilla.org/mozilla-central/rev/a06b6d9f6540
https://hg.mozilla.org/mozilla-central/rev/450ea0e5c8cb
https://hg.mozilla.org/mozilla-central/rev/b2ae0fa93760
https://hg.mozilla.org/mozilla-central/rev/fc13616aed73
https://hg.mozilla.org/mozilla-central/rev/f7d9523b227c
https://hg.mozilla.org/mozilla-central/rev/830dc8e6e370
https://hg.mozilla.org/mozilla-central/rev/85fc8ee6d839
https://hg.mozilla.org/mozilla-central/rev/28226109634e
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 61•9 years ago
|
||
Updated following documents
https://developer.mozilla.org/en-US/Firefox/Releases/48
https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_6_support_in_Mozilla
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/@@species
Updated•6 years ago
|
Depends on: CVE-2019-9810
Updated•3 years ago
|
No longer depends on: CVE-2019-9810
Regressions: CVE-2019-9810
You need to log in
before you can comment on or make changes to this bug.
Description
•