1177318 - Template strings dramatically slower than string concatenation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kevin Decker]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/43.0.2357.130 Safari/537.36

Steps to reproduce:

1. [e-mail]:kpdecker/six-speed.git
2. npm install
3. gulp server
4. open http://localhost:9999/#template


Actual results:

Rendering the string was 600x slower than equivalent operations that manually concatenate the fields.

See template string section of http://kpdecker.github.io/six-speed/


Expected results:

Performance at or above the ES5 equivalent.

Comment 1

[Jason Orendorff [:jorendorff]]

This is because Ion doesn't have any support for JSOP_TOSTRING. I have a patch for this, but it raises some questions.

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Add Ion support for JSOP_TOSTRING, to make template strings faster

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8646993 [details] [diff] [review]
Add Ion support for JSOP_TOSTRING, to make template strings faster

I guess I will go ahead and request review on this.

I tried to run six-speed locally, but after going to localhost:9999, I just get:

    {"statusCode":404,"error":"Not Found"}

Anyway, this patch seems to fix the issue, but:

1. As always with my JIT patches, this was developed by assertion roulette. I left one "???" in the patch.

2. This makes template strings about 25% faster than regular strings in this particular case. I don't understand why. It seems unlikely to be good; they should be the same speed.

3. I also found a variation of this test where whichever test runs first is super fast, but all subsequent runs of *either* the ES5 or the ES6 code are about 100x slower. Also seems bad. Here it is:

function test(f) {
    var t0 = Date.now();
    for (var i = 0; i < 10000000; i++) {
        f();
    }
    print(Date.now() - t0);
}

function es5() {
    var data = [1,2,3];
    function f5() {
        return data[0] + ' ' + (data[1] + data[2]);
    }
    assertEq(f5(), '1 5');
    test(f5);
}

function es6() {
    var data = [1,2,3];
    function f6() {
        return `${data[0]} ${data[1] + data[2]}`;
    }
    assertEq(f6(), '1 5');
    test(f6);
}

es6();
es5();
es6();
es5();
es6();
es5();

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8646993 [details] [diff] [review]
Add Ion support for JSOP_TOSTRING, to make template strings faster

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

Looks good, thanks!

::: js/src/jit/IonBuilder.cpp
@@ +634,5 @@
>              continue;
>  
>          MPhi* phi = entry->getSlot(slot)->toPhi();
>  
> +        if (*last == JSOP_POS)  // ??? what is this? should I do something special for TOSTRING here?

Can just ignore this. I think it's a hack to give JSOP_POS the type of its input, for TOSTRING the result is always a string and that's handled below.

@@ +4676,5 @@
> +    MToString* ins = MToString::New(alloc(), value);
> +    current->add(ins);
> +    current->push(ins);
> +    if (ins->isEffectful())
> +        return resumeAfter(ins);

We can remove the last two lines because MToString is never effectful atm.

MToString currently bails when the input is an object. That's pretty bad because it can lead to unnecessary bailing. I can file a followup bug to fix that once this lands.

I can also look into the remaining performance difference. I agree it's unexpected.

Comment 5

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8897f109a08a2965497a7407f061b5009c19359
Bug 1177318 - Add Ion support for JSOP_TOSTRING, to make template strings faster. r=jandem.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out (along with everything else from that push) for being the likely cause of hazard build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=14590837&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/027ddfe2c4af

Comment 7

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1769e6d4a5d4a7e6f25b7e2b03d5994df1123e9e
Bug 1177318 - Add Ion support for JSOP_TOSTRING, to make template strings faster. r=jandem.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1769e6d4a5d4