Last Comment Bug 1177318 - Template strings dramatically slower than string concatenation
: Template strings dramatically slower than string concatenation
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 38 Branch
: Unspecified Unspecified
-- normal with 2 votes (vote)
: mozilla44
Assigned To: Jason Orendorff [:jorendorff]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 1185046 (view as bug list)
Depends on: 1180184 1185046
Blocks: six-speed
  Show dependency treegraph
 
Reported: 2015-06-24 19:59 PDT by Kevin Decker
Modified: 2015-10-06 07:35 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Add Ion support for JSOP_TOSTRING, to make template strings faster (5.65 KB, patch)
2015-08-12 09:12 PDT, Jason Orendorff [:jorendorff]
jdemooij: review+
Details | Diff | Splinter Review

Description User image Kevin Decker 2015-06-24 19:59:40 PDT
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.
Comment 1 User image Jason Orendorff [:jorendorff] 2015-08-12 09:12:30 PDT
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 User image Jason Orendorff [:jorendorff] 2015-08-12 09:12:44 PDT
Created attachment 8646993 [details] [diff] [review]
Add Ion support for JSOP_TOSTRING, to make template strings faster
Comment 3 User image Jason Orendorff [:jorendorff] 2015-08-12 10:27:17 PDT
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 User image Jan de Mooij [:jandem] 2015-08-17 04:24:33 PDT
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 User image Jason Orendorff [:jorendorff] 2015-09-23 10:20:11 PDT
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 User image Wes Kocher (:KWierso) 2015-09-23 12:32:43 PDT
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 User image Jason Orendorff [:jorendorff] 2015-09-30 08:37:07 PDT
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 User image Carsten Book [:Tomcat] 2015-10-01 05:55:41 PDT
https://hg.mozilla.org/mozilla-central/rev/1769e6d4a5d4
Comment 9 User image Tom Schuster [:evilpie] 2015-10-06 07:35:49 PDT
*** Bug 1185046 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.