Small TypedArray constructor clean-ups

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(7 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Bug to track some small code changes for TypedArray constructors.
(Assignee)

Comment 1

2 years ago
This changes AllocateArrayBuffer's |ctor| argument to HandleObject, which allows us to skip from Object<->Value transitions.
Attachment #8900331 - Flags: review?(jdemooij)
(Assignee)

Comment 2

2 years ago
This skips the "prototype" property lookup for the built-in ArrayBuffer constructor, which makes the TypedArray "copy-constructor" (https://tc39.github.io/ecma262/#sec-typedarray-typedarray) a bit faster.
Attachment #8900334 - Flags: review?(jdemooij)
(Assignee)

Comment 3

2 years ago
Neither initTypedArraySlots nor initTypedArrayData can GC, so we don't need to root |obj| and instead can simply assign the typed array to the result variable.
Attachment #8900336 - Flags: review?(jdemooij)
(Assignee)

Comment 4

2 years ago
Removes an unused JSContext* argument from ElementSpecific and initTypedArraySlots.
Attachment #8900337 - Flags: review?(jdemooij)
Attachment #8900331 - Flags: review?(jdemooij) → review+
Comment on attachment 8900334 [details] [diff] [review]
bug1393089-part2-prototype-lookup.patch

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

::: js/src/vm/TypedArrayObject.cpp
@@ +1056,2 @@
>  
> +    // As an optimization, skip the "prototype" lookup for %

Nit: finish the comment. The spec calls this %ArrayBuffer% I guess?
Attachment #8900334 - Flags: review?(jdemooij) → review+
Attachment #8900336 - Flags: review?(jdemooij) → review+
Comment on attachment 8900337 [details] [diff] [review]
bug1393089-part4-unused-context.patch

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

Nice patches again, thanks!
Attachment #8900337 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 7

2 years ago
(In reply to Jan de Mooij [:jandem] from comment #5)
> Comment on attachment 8900334 [details] [diff] [review]
> bug1393089-part2-prototype-lookup.patch
> 
> Review of attachment 8900334 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/TypedArrayObject.cpp
> @@ +1056,2 @@
> >  
> > +    // As an optimization, skip the "prototype" lookup for %
> 
> Nit: finish the comment. The spec calls this %ArrayBuffer% I guess?

Ups, copy-paste error. The rest is a few lines above: "// 24.1.1.1 step 1 (partially).ArrayBuffer%." :-)
(Assignee)

Comment 8

2 years ago
Part 1 depends on bug 1386534, which got recently backed out, so marking it as a blocker for now.
Depends on: 1386534
(Assignee)

Comment 9

2 years ago
Update part 2 to fix comment. Carrying r+.
Attachment #8900334 - Attachment is obsolete: true
Attachment #8901220 - Flags: review+
(Assignee)

Comment 10

2 years ago
Another small clean-up for typed array constructors.
Attachment #8901841 - Flags: review?(jdemooij)
Comment on attachment 8901841 [details] [diff] [review]
bug1393089-part5-unused-variable-rooting.patch

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

LGTM.
Attachment #8901841 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 12

2 years ago
While we wait for bug 1386534 to re-land, we can add more clean-ups (this patch) and optimizations (the next patch).

This updates the TypedArray constructor code to more closely follow the updated steps from https://github.com/tc39/ecma262/commit/7fa0bb2cb7b726a85afa8e4c00fe1bebffb706bf.

I've removed the CloneArrayBufferNoCopy method, because it no longer seems useful to have a separate method for just two steps (the detached-check and the call to AllocateArrayBuffer), and because this allows us to share the detached-check after AllocateArrayBuffer for both code paths. And yes, the detached-checks are non-uniform per the current spec (https://github.com/tc39/ecma262/pull/844#issue-213044293).
Attachment #8906262 - Flags: review?(jdemooij)
(Assignee)

Comment 13

2 years ago
With this optimization we no longer need to reify the array buffer object when copying TypedArrays.

It improves this µ-benchmark from ~245ms to ~155ms for me:

    var q = 0;
    var dt = 0;
    for (var i = 0; i < 100000; ++i) {
        var src = new Int32Array(100);

        var t = dateNow();
        q += new Int32Array(src).length;
        dt += dateNow() - t;
    }
    print(dt);
Attachment #8906264 - Flags: review?(jdemooij)
Attachment #8906262 - Flags: review?(jdemooij) → review+
Comment on attachment 8906264 [details] [diff] [review]
bug1393089-part7-lazy-buffer-in-constructor.patch

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

Nice.
Attachment #8906264 - Flags: review?(jdemooij) → review+

Comment 16

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/daaddc22f93f
Part 1: Change AllocateArrayBuffer constructor argument to HandleObject. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac55815c6bc
Part 2: Avoid "prototype" lookup for ArrayBuffer when copying typed arrays. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5a9cdfcffa6
Part 3: Directly assign result to variable instead of going through another temp variable. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e943b6a9520
Part 4: Remove unused context arguments from ElementSpecific and initTypedArraySlots. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cc7890ba6e3
Part 5: Remove no longer used variable and avoid one unnecessary rooting. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/4acbcf36de47
Part 6: Reduce code duplications by sync'ing TypedArray constructor to latest spec. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/453483778b6e
Part 7: Don't force source buffer allocation when copying TypedArrays. r=jandem
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.