Closed
Bug 1125509
Opened 10 years ago
Closed 10 years ago
10% tsvgx linux* regression on inbound (Jan 22) from push c6ece3462afd
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
INVALID
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: jmaher, Assigned: jmaher)
References
Details
(Keywords: perf, regression, Whiteboard: [talos_regression][dzalerts][gfx-noted])
Attachments
(1 file)
1.53 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
Flags: needinfo?(nfroyd)
![]() |
||
Comment 1•10 years ago
|
||
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•10 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).
![]() |
||
Comment 3•10 years ago
|
||
(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•10 years ago
|
||
(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)
![]() |
||
Updated•10 years ago
|
Whiteboard: [talos_regression][dzalerts] → [talos_regression][dzalerts][gfx-noted]
Assignee | ||
Comment 5•10 years ago
|
||
friendly ping here
Comment 6•10 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)
Comment 7•10 years ago
|
||
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)
Comment 8•10 years ago
|
||
I think we should just back it out until we figure out what's going on here.
Flags: needinfo?(jmuizelaar)
Comment 9•10 years ago
|
||
(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•10 years ago
|
||
Attachment #8562081 -
Flags: review?(nfroyd)
![]() |
||
Updated•10 years ago
|
Attachment #8562081 -
Flags: review?(nfroyd) → review+
Comment 11•10 years ago
|
||
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.
Assignee | ||
Comment 12•10 years ago
|
||
Comment 13•10 years ago
|
||
Assignee: nobody → jmaher
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee | ||
Comment 15•10 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)
Comment 16•10 years ago
|
||
(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.
Description
•