Closed Bug 1273955 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: h4writer, Assigned: nbp)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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
Blocks: 1261826
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 :/
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
(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.
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.
Attachment #8754432 - Flags: review?(hv1989)
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
Attachment #8754432 - Flags: review?(hv1989) → review+
(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!
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
(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.
Flags: needinfo?(philringnalda)
As pointed by ochameau, this might be related to Bug 1266676.
Flags: needinfo?(philringnalda)
Depends on: 1276964
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.
(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)
Depends on: 1115779
Pushed by npierron@mozilla.com:
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
(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.
https://hg.mozilla.org/mozilla-central/rev/1e841da3ed13
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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 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
Attachment #8754432 - Flags: approval-mozilla-aurora?
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.
Attachment #8754432 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.