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

Status: RESOLVED INVALID

Description

[Joel Maher ( :jmaher ) (UTC-4)]

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.

Comment 1

[Nathan Froyd [:froydnj] (away 18 Oct - 21 Oct)]

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?

Comment 2

[Avi Halachmi (:avih)]

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).

Comment 3

[Nathan Froyd [:froydnj] (away 18 Oct - 21 Oct)]

(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.

Comment 4

[Jonathan Watt [:jwatt]]

(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.

Comment 5

[Joel Maher ( :jmaher ) (UTC-4)]

friendly ping here

Comment 6

[Avi Halachmi (:avih)]

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?

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 8

[Jeff Muizelaar [:jrmuizel]]

I think we should just back it out until we figure out what's going on here.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 10

[Joel Maher ( :jmaher ) (UTC-4)]

Created attachment 8562081 [details] [diff] [review]
backout 70aa258394bf (1.0)

Comment 11

[Avi Halachmi (:avih)]

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.

Comment 12

[Joel Maher ( :jmaher ) (UTC-4)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc194ba77d7f

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dc194ba77d7f

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Did this improve?

Comment 15

[Joel Maher ( :jmaher ) (UTC-4)]

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.

Comment 16

[Avi Halachmi (:avih)]

(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).