1273955 - 12% regression on octane-richards on May 17th 2016

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

Bug 1261826 (branch pruning) decreases performance of octane richards on x86 and x64 with 12%. 

https://arewefastyet.com/#machine=28&view=single&suite=octane&subtest=Richards&start=1463498148&end=1463522501

Comment 1

[Hannes Verschore [:h4writer]]

And interestingly 5% on mandelbrot-polyfill (asmjs)
https://arewefastyet.com/#machine=28&view=single&suite=asmjs-ubench&subtest=mandelbrot-polyfill&start=1463494307&end=1463525627

Comment 2

[Jan de Mooij [:jandem]]

It's probably the bytecode size threshold we use for "small functions". When I changed the bytecode for the `this` refactoring, I also regressed richards and had to bump this limit a little :/

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #1)
> And interestingly 5% on mandelbrot-polyfill (asmjs)
> https://arewefastyet.com/#machine=28&view=single&suite=asmjs-
> ubench&subtest=mandelbrot-polyfill&start=1463494307&end=1463525627

Looking at the difference of this benchmarks as this was the most unexpected.
The issue comes from ecmascript_simd.js:462 which is the polyfill for SIMD.int32x4.

The bytecode size changed from 119 bytes to 122 bytes. (which is even less than the 5.8% mentioned in Bug 1261826 comment 36)

I will bump the small function bytecode size limit, and see if this also fix the Richards regression.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Bump the bytecode size limit of Ion to account for increased size of JSScript code length.

Increasing the small function max bytecode length was enough to solves the
polyfill issue, but this was not enough for Richards.  Bumping the small
function max bytecode length to 127 was enough to fix half of the
performance issue on Richards.

The rest of Richards issue was caused by the inaibility to inlined
previously inlined functions.  Thus In rounded up all the numbers before
multiplying them by 1.058.

This patch solves both the polyfill and Richards issue.

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 8754432 [details] [diff] [review]
Bump the bytecode size limit of Ion to account for increased size of JSScript code length.

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

Makes sense

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16a18faa5363

Comment 7

[Hannes Verschore [:h4writer]]

(In reply to Pulsebot from comment #6)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/16a18faa5363

Confirmed fixed on AWFY. Both regressions are gone with this revision. Thanks!

Comment 8

[Phil Ringnalda (:philor)]

You came awfully close to being able to sneak that in - bug 1115779 just got fixed last Friday, and we never would have noticed your patch adding to the already extreme frequency of browser_wa_destroy-node-01.js | Test timed out before it was fixed. Now that it's fixed, though...

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a94bd69a8616 for frequent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=28477894&repo=mozilla-inbound

Comment 9

[Nicolas B. Pierron [:nbp]]

(In reply to Phil Ringnalda (:philor) from comment #8)
> You came awfully close to being able to sneak that in - bug 1115779 just got
> fixed last Friday, and we never would have noticed your patch adding to the
> already extreme frequency of browser_wa_destroy-node-01.js | Test timed out
> before it was fixed. Now that it's fixed, though...

I can see only 2 options, either:
 1/ This is a latent bug in the JS engine.
 2/ The test case is not completely fixed.

In any case, I do not see how this patch should change this behaviour knowing that this patch restores the behaviour which used to be present before Wednesday, when Bug 1261826 landed.

Somehow, I can bet that we might have the same issue if we were to backout Bug 1261826, which is part of the remaining options if we cannot land this patch.

Comment 10

[Nicolas B. Pierron [:nbp]]

As pointed by ochameau, this might be related to Bug 1266676.

Comment 11

[Nicolas B. Pierron [:nbp]]

I am currently investigating ways the devtool intermittent issue to get more information.

So far, I managed to reproduce it with IONGRAPH=scripts,bailouts,bl-bails but not with IONFLAGS=logs (probably due to the extra latency added by the logging).  I also tried to add more "dump" calls inside the last d8.js function from which we bailed out, and the intermittent disappeared.  My guess is that this look like a synchronization issue between threads, but I still do not udnerstand how the JIT heuristics can change any of that, except by changing the way others threads are scheduled by the system.

I started collecting TraceLogger data, but due to the fact that we spawn an xpcshell and a browser, I seem to only get trace for the xpcshell, but not for the browser.  (I see httpd.js, but not d3.js)  I have no clue on how to add environment variable only to the browser, but I guess I will hack my way around.

Comment 12

[Hannes Verschore [:h4writer]]

(In reply to Nicolas B. Pierron [:nbp] from comment #11)
> I started collecting TraceLogger data, but due to the fact that we spawn an
> xpcshell and a browser, I seem to only get trace for the xpcshell, but not
> for the browser.  (I see httpd.js, but not d3.js)  I have no clue on how to
> add environment variable only to the browser, but I guess I will hack my way
> around.

You might want to look at /tmp/.
You'll see tl-data.json which contains a link to the log of the last process created.
Though the other processes also get logged. They are named tl-data.XXPIDXX.json.
If you point the website to tl-data.XXPIDXX.json you will see the info for that process.
(by appending ?data=tl-data.XXPIDXX.json to the url)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e841da3ed13
Bump the bytecode size limit of Ion to account for increased size of JSScript code length. r=h4writer

Comment 14

[Nicolas B. Pierron [:nbp]]

(In reply to Phil Ringnalda (:philor) from comment #8)
> You came awfully close to being able to sneak that in - bug 1115779 just got
> fixed last Friday, […]

Note, I just re-landed the same patch as-is, because Bug 1115779 was fixed in a broken way, and since the bug got re-opened and re-closed.

Comment 15

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

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

Comment 16

[Hannes Verschore [:h4writer]]

Alert of the improvements:
https://treeherder.allizom.org/perf.html#/alerts?id=2608

Seems the richards regression is gone. Not sure about the other regressions now introduced. First of all most of these regressions have an improvement that date to earlier branch pruning patches. Feel free to take a deeper look, but I'm not intending to track them. Except if others think they are important

Comment 17

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8754432 [details] [diff] [review]
Bump the bytecode size limit of Ion to account for increased size of JSScript code length.

Approval Request Comment
[Feature/regressing bug #]: Bug 1261826
[User impact if declined]: Performance regression.
[Describe test coverage new/current, TreeHerder]: Landed in m-c
[Risks and why]: Low.
[String/UUID change made/needed]: N/A

Note:
For backporting this patch to aurora we might have to backport the latest patch from Bug 1115779 (*).

(*) https://hg.mozilla.org/mozilla-central/rev/815b7fb28fdf

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8754432 [details] [diff] [review]
Bump the bytecode size limit of Ion to account for increased size of JSScript code length.

Perf regression from 49, ok to uplift. 
Looks like most of bug 1261826 landed in 49 already.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/839310c1fc4f