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

RESOLVED FIXED in Firefox 49

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: h4writer, Assigned: nbp)

Tracking

(Depends on: 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed, firefox50 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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)

Updated

3 years ago
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)

Updated

3 years ago
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 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

3 years ago
Created attachment 8754432 [details] [diff] [review]
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.
Attachment #8754432 - Flags: review?(hv1989)
(Reporter)

Comment 5

3 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

3 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!
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

3 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

3 years ago
As pointed by ochameau, this might be related to Bug 1266676.
Flags: needinfo?(philringnalda)
(Assignee)

Updated

3 years ago
Depends on: 1276964
(Assignee)

Comment 11

3 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

3 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

3 years ago
status-firefox49: --- → affected
status-firefox50: --- → affected
(Reporter)

Updated

3 years ago
Depends on: 1115779

Comment 13

3 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

3 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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1e841da3ed13
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(Reporter)

Comment 16

3 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

3 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 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

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/839310c1fc4f
status-firefox49: affected → fixed
You need to log in before you can comment on or make changes to this bug.