1381474 - Replace detached buffer check with typed array length check when sorting typed arrays with a custom comparator

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, enhancement)

Description

[André Bargull [:anba]]

As described in bug 1381469, comment #0, it's currently faster for us to check the current typed array length than to check for a detached buffer.

Improves this µ-benchmark from 800ms to 150ms for me:

    var n = 100;
    var ta = new Int32Array(n);
    var q = 0;
    var t = 0;
    for (var i = 0; i < 100000; ++i) {
        for (var j = 0; j < n; ++j) ta[j] = n - j;
        dt = dateNow();
        ta.sort((a, b) => a - b);
        t += dateNow() - dt;
        q += ta[0];
    }
    print(t, q);

Comment 1

[André Bargull [:anba]]

Attachment: bug1381474.patch

I had to update the test file, because the TypeError is now thrown from the correct global (callee global instead of |this-value| global).

Comment 2

[Till Schneidereit [:till]]

Comment on attachment 8887035 [details] [diff] [review]
bug1381474.patch

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

I wonder if it'd make sense to hoist the isTypedArray check out of wrappedCompareFn. I.e., should we use different wrappers depending isTypedArray? That'd probably make things slower for code that uses TA.p.sort on both unwrapped *and* wrapped objects, but faster for other code.

Anyway, not fodder for this bug, clearly.

Comment 3

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d929d586209513f21fe082c7d03f2e4e97c43e68

Comment 4

[André Bargull [:anba]]

(In reply to Till Schneidereit [:till] from comment #2)
> I wonder if it'd make sense to hoist the isTypedArray check out of
> wrappedCompareFn. I.e., should we use different wrappers depending
> isTypedArray? That'd probably make things slower for code that uses
> TA.p.sort on both unwrapped *and* wrapped objects, but faster for other code.
> 
> Anyway, not fodder for this bug, clearly.

Hmm, so I just tried this and for whatever reason, Ion decided to generate worse code (doubled execution time for a simple sort benchmark) when I removed the if-statement from the comparator wrapper. I guess I just keep the current version for now... ;-)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1079b8daff1
Check for detached buffers by querying the current length when sorting typed arrays. r=till

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c1079b8daff1