10% tsvgx linux* regression on inbound (Jan 22) from push c6ece3462afd

RESOLVED INVALID

Status

()

RESOLVED INVALID
4 years ago
4 years ago

People

(Reporter: jmaher, Assigned: jmaher)

Tracking

({perf, regression})

Trunk
mozilla38
All
Linux
perf, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [talos_regression][dzalerts][gfx-noted])

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Oddly enough alert manager didn't catch this, but we saw it from our new system that we will be integrating into treeherder in the coming months.

this only affects linux* and as far as I know tsvgx:
http://graphs.mozilla.org/graph.html#tests=[[281,131,33],[281,131,35]]&sel=none&displayrange=7&datatype=geo

on treeherder, I did some retriggers:
https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&filter-searchStr=Ubuntu HW 12.04 mozilla-inbound talos svgr&fromchange=068f4ad023db&tochange=568bbf124221

on this change we went from ~345 -> ~320:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=c6ece3462afd

To test this patch on try you can use this syntax:
try: -b o -p linux,linux64 -u none -t svgr

To run talos locally here are instructions:
https://wiki.mozilla.org/Buildbot/Talos/Running#Running_locally_-_Source_Code

you can run it like this:
./talos -e <firefox> -a tsvgx --develop

Here is more information about the test itself:
https://wiki.mozilla.org/Buildbot/Talos/Tests#tsvg.2C_tsvgx


As the patch author we need your feedback to help us determine what the next steps are:
* If this doesn't seem possible that it could be related to your bug, let us know ASAP
* If this is possible that you caused a regression, but not all of this, let us know ASAP
* Otherwise, we would like to know by Tuesday if:
** this is expected, and we should accept the regression. Please provide documentation and we will close this bug
** this is unexpected, but is limited in scope and scale- to investigate and fix this would be not worthwhile based on how long it would take to sort out
** this is unexpected, but after a brief look at the code it is likely you could fix this; do let us know and provide a general timeline
** this is unexpected and we are not sure what to do, we should backout for now.
(Assignee)

Updated

4 years ago
Flags: needinfo?(nfroyd)
I think this is almost certainly bug 967300; I have a hard time seeing how bug 1122235 could cause this, and bug 1122126 doesn't come anywhere near SVG rendering code.

The problem is that the fix for bug 967300 has nice knock-on effects, namely enabling the pattern pool in cairo; bug 1111885 comment 3 suggests that we ought to be using the cairo pattern pool all the time.  Bug 1111885 is a case where we have a lot of allocations being seen in some workloads, and those allocations completely disappear after bug 967300 enabled cairo's pattern pool.

I also have a hard time knowing how cached patterns could make things more expensive for this test...but then I don't have a lot of knowledge of our graphics stack, SVG, or cairo.  Jonathan, your name is attached to tsvgx on the talos page; do you have any guesses as to how bits of cairo could cause this svg regression?
Flags: needinfo?(nfroyd) → needinfo?(jwatt)

Comment 2

4 years ago
Note that if you guys think that this regression is a reasonable price to pay for whatever other advantages the patch brings, then that's usually fine.

We just need someone who's familiar with the subject to say so (or, of course, if it's unexpected or if you'd like to improve it - that's also fine).
(In reply to Avi Halachmi (:avih) from comment #2)
> Note that if you guys think that this regression is a reasonable price to
> pay for whatever other advantages the patch brings, then that's usually fine.
> 
> We just need someone who's familiar with the subject to say so (or, of
> course, if it's unexpected or if you'd like to improve it - that's also
> fine).

Yeah, that's not me, hence the ni? on Jonathan.
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #1)
> Jonathan, your name is attached to tsvgx on
> the talos page; do you have any guesses as to how bits of cairo could cause
> this svg regression?

No, someone would have to profile it. Unless Jeff knows?

In general though it sounds like the benefits outweigh the cost, so if we can't figure out how to avoid this hit, I guess we just take it.
Flags: needinfo?(jwatt) → needinfo?(jmuizelaar)
Whiteboard: [talos_regression][dzalerts] → [talos_regression][dzalerts][gfx-noted]
(Assignee)

Comment 5

4 years ago
friendly ping here

Comment 6

4 years ago
It seems we find it hard to decide if this regression is acceptable or not.

Milan, can someone at the gfx team state if this is acceptable, and if not, how and when it will be handled?
Flags: needinfo?(milan)
Yeah, Jeff is the right person to look and decide on this, but he's a bit under the water with some short term 36 issues - let's give this the benefit of the doubt for this train and revisit the next one?
Flags: needinfo?(milan)
I think we should just back it out until we figure out what's going on here.
Flags: needinfo?(jmuizelaar)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> I think we should just back it out until we figure out what's going on here.

I can do some investigation when I get back to to Toronto next week.
(Assignee)

Comment 10

4 years ago
Created attachment 8562081 [details] [diff] [review]
backout 70aa258394bf (1.0)
Attachment #8562081 - Flags: review?(nfroyd)
Attachment #8562081 - Flags: review?(nfroyd) → review+
I think we should wait till next week and let jeff answer if the regression is acceptable or not.

The patch seems to me small enough to back out later with same ease as backing it out now, and backing it out now will introduce more variance into the test results.
https://hg.mozilla.org/mozilla-central/rev/dc194ba77d7f
Assignee: nobody → jmaher
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Did this improve?
Flags: needinfo?(jmaher)
(Assignee)

Comment 15

4 years ago
I see no changes there, but somehow I have to question the original regression, in fact it looked like an improvement, I know avih recently rewrote svg to be asap, I assume lower is still better.

If that is the case this should have not been filed.
Flags: needinfo?(jmaher) → needinfo?(avihpit)
(In reply to Joel Maher (:jmaher) from comment #0)
> on this change we went from ~345 -> ~320:

(In reply to Joel Maher (:jmaher) from comment #15)
> ... I assume lower is still better.
> If that is the case this should have not been filed.

Meh, I completely missed the fact that the numbers got lower. This indeed an improvement.

AFAIK, all the talos tests are "lower is better", except Dormaeo, Canvasmark and V8 - where higher is better.

(it's almost all the "non-talos" tests, but Kraken is apparently better when lower).
Flags: needinfo?(avihpit)
Resolution: FIXED → INVALID
You need to log in before you can comment on or make changes to this bug.