The default bug view has changed. See this FAQ.

Template strings dramatically slower than string concatenation

RESOLVED FIXED in Firefox 44

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Kevin Decker, Assigned: jorendorff)

Tracking

(Blocks: 1 bug)

38 Branch
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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. git@github.com: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.

Updated

2 years ago
Blocks: 1177735
Depends on: 1180184
Component: Untriaged → JavaScript Engine
Product: Firefox → Core
Depends on: 1185046
(Assignee)

Comment 1

2 years ago
This is because Ion doesn't have any support for JSOP_TOSTRING. I have a patch for this, but it raises some questions.
(Assignee)

Comment 2

2 years ago
Created attachment 8646993 [details] [diff] [review]
Add Ion support for JSOP_TOSTRING, to make template strings faster
(Assignee)

Updated

2 years ago
Assignee: nobody → jorendorff
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 3

2 years ago
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();
Attachment #8646993 - Flags: review?(jdemooij)
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.
Attachment #8646993 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 5

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8897f109a08a2965497a7407f061b5009c19359
Bug 1177318 - Add Ion support for JSOP_TOSTRING, to make template strings faster. r=jandem.
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
(Assignee)

Comment 7

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1769e6d4a5d4a7e6f25b7e2b03d5994df1123e9e
Bug 1177318 - Add Ion support for JSOP_TOSTRING, to make template strings faster. r=jandem.
https://hg.mozilla.org/mozilla-central/rev/1769e6d4a5d4
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Duplicate of this bug: 1185046
You need to log in before you can comment on or make changes to this bug.