1430800 - Optimize String.raw by using arguments object instead of rest-arguments

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

Using the arguments object instead of rest-arguments seems to generate faster code when String.raw is called with substitutions. 

µ-benchmarks I've used for testing: (The String.raw calls always compute empty strings to avoid benchmarking string allocations. Hopefully this doesn't perturb the results in any way.)
---
// zeroSubst: 20ms -> 20ms
// oneSubst: 370ms -> 125ms
// twoSubst: 450ms -> 215ms

function zeroSubst() {
    var q = 0;
    var t = dateNow();
    for (var i = 0; i < 10000000; ++i) {
        q += String.raw ``.length;
    }
    return [dateNow() - t, q];
}

function oneSubst() {
    var xs = ["", ""];
    var q = 0;
    var t = dateNow();
    for (var i = 0; i < 10000000; ++i) {
        q += String.raw `${xs[i & 1]}`.length;
    }
    return [dateNow() - t, q];
}

function twoSubst() {
    var q = 0;
    var t = dateNow();
    for (var i = 0; i < 10000000; ++i) {
        q += String.raw `${xs[i & 1]}${xs[i & 1]}`.length;
    }
    return [dateNow() - t, q];
}
---

Comment 1

[André Bargull [:anba]]

Attachment: bug1430800.patch

Jan, do you think it's worthwhile to change the String.raw implementation to avoid rest-parameters or is there anything we can do in the JITs to achieve a similar performance improvement like with this patch?

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8942927 [details] [diff] [review]
bug1430800.patch

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

Good find! Yeah this makes sense because we optimize |arguments| more than rest parameters atm.

Comment 3

[André Bargull [:anba]]

Attachment: bug1430800.patch

This patch replaces the rest-parameter usage in String.raw with an arguments object based on the performance results in comment #0. 

I've also updated the step comments, changed the comparison in step 6 from |literalSegments <= 0| to |literalSegments === 0| (I'll create a spec PR to upstream this change), and added a special-case for |literalSegments === 1| to avoid a performance regression with the new code when String.raw is called with a single literal segment, i.e. String.raw `<literal>`.

Comment 4

[Tom S [:evilpie]]

Comment on attachment 8943612 [details] [diff] [review]
bug1430800.patch

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

Nice improvement

Comment 5

[André Bargull [:anba]]

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e5c359b9fe3
Optimize String.raw by avoiding rest-parameter array allocation. r=evilpie

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/1e5c359b9fe3