Closed
Bug 1273955
Opened 9 years ago
Closed 9 years ago
12% regression on octane-richards on May 17th 2016
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: h4writer, Assigned: nbp)
References
(Depends on 1 open bug)
Details
Attachments
(1 file)
2.40 KB,
patch
|
h4writer
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•9 years ago
|
||
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•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•9 years ago
|
||
(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.
Assignee | ||
Comment 4•9 years ago
|
||
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)
Reporter | ||
Comment 5•9 years ago
|
||
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+
Reporter | ||
Comment 7•9 years ago
|
||
(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•9 years ago
|
||
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
Assignee | ||
Comment 9•9 years ago
|
||
(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)
Assignee | ||
Comment 10•9 years ago
|
||
As pointed by ochameau, this might be related to Bug 1266676.
Updated•9 years ago
|
Flags: needinfo?(philringnalda)
Assignee | ||
Comment 11•9 years ago
|
||
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.
Reporter | ||
Comment 12•9 years ago
|
||
(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)
Reporter | ||
Updated•9 years ago
|
status-firefox49:
--- → affected
status-firefox50:
--- → affected
Comment 13•9 years ago
|
||
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
Assignee | ||
Comment 14•9 years ago
|
||
(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•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Reporter | ||
Comment 16•9 years ago
|
||
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
Assignee | ||
Comment 17•9 years ago
|
||
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 18•9 years ago
|
||
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+
Comment 19•9 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•