Closed
Bug 935008
Opened 11 years ago
Closed 11 years ago
SVG Talos tests have regressed due to bug 922942
Categories
(Core :: Graphics, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox27 | --- | unaffected |
firefox28 | + | fixed |
People
(Reporter: jmaher, Assigned: jwatt)
References
Details
(Keywords: perf, regression, Whiteboard: [c=automation p= s=2013.12.06 u=1.3] [talos_regression][qa-])
With the landing of bug 922942 we have regressed our svg tests by 10-25% depending on the specific test and platform.
It is my understanding that there is a secondary bug that will fix the majority of this regression (bug 934183) which should land before the next uplift.
Comment 1•11 years ago
|
||
For reference:
https://groups.google.com/d/topic/mozilla.dev.tree-management/8caTImH3_9o/discussion
Mozilla-Inbound-Non-PGO - SVG, Opacity Row Major - WINNT 6.1 (ix) - 83.9% increase
----------------------------------------------------------------------------------
Previous: avg 238.458 stddev 4.234 of 12 runs up to revision 56fc3fc6a42a
New : avg 438.500 stddev 3.949 of 12 runs since revision a60b5eb6b8ba
Change : +200.042 (83.9% / z=47.242)
Graph : http://mzl.la/173eIgo
Regression: Mozilla-Inbound-Non-PGO - SVG-ASAP - WINNT 6.1 (ix) - 155% increase
-------------------------------------------------------------------------------
Previous: avg 362.428 stddev 3.110 of 12 runs up to revision 56fc3fc6a42a
New : avg 925.322 stddev 7.674 of 12 runs since revision a60b5eb6b8ba
Change : +562.894 (155% / z=181.018)
Graph : http://mzl.la/173eGoK
Blocks: 922942
status-firefox27:
--- → unaffected
status-firefox28:
--- → affected
tracking-firefox28:
--- → ?
No longer depends on: 922942
Keywords: perf,
regression
Summary: SVG Talos tests have regressed → SVG Talos tests have regressed due to bug 922942
Whiteboard: [perf_regression]
Version: unspecified → 28 Branch
Comment 2•11 years ago
|
||
mattwoodrow: Triage is trying to gauge the impact this will have can you give your thoughts since you worked on 922942. Were we expecting a regression like this?
Flags: needinfo?(matt.woodrow)
Comment 3•11 years ago
|
||
Yes, this is definitely expected.
The problem is that Thebes and Moz2D have very different semantics when it comes to storing path data. My patches left SVG making calls that are optimized for the Thebes API, but manually maps to the Moz2D API in the backend. This added quite a bit of extra overhead (though admittedly more than I expected).
Jonathan Watt is working on patches that convert SVG to use the Moz2D API directly, which should all of this regression. We're expecting these patches to land before the uplift.
Flags: needinfo?(matt.woodrow)
Comment 4•11 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
>
> Jonathan Watt is working on patches that convert SVG to use the Moz2D API
> directly, which should all of this regression. We're expecting these patches
> to land before the uplift.
Nice! I'm going to tag this as 1.3 so that we can keep track of the progress, as we're actually looking to improve SVG performance for 1.3, rather than just get back to where we were.
Assignee: nobody → jwatt
blocking-b2g: --- → 1.3+
Reporter | ||
Updated•11 years ago
|
Whiteboard: [talos_regression]
Reporter | ||
Comment 5•11 years ago
|
||
there was a SVG improvement landed yesterday, I don't see action on bug 934183, but it appears to be one of these two commits:
roc, bug 931464, bug 933354:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=abda071349c4
jwatt, bug 934156, bug 926564, bug 866659:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f2f5e3c75e15
Can we confirm that one of these bugs did a slight improvement to svg tests:
https://datazilla.mozilla.org/?start=1383322827&stop=1383927627&product=Firefox&repository=Mozilla-Inbound-Non-PGO&os=win&os_version=6.1.7601&test=tsvgx&project=talos
For the record we haven't removed all the regressions from earlier in the week, but we have made measurable improvements.
Reporter | ||
Comment 6•11 years ago
|
||
for reference, you can see the improvement across different platforms here:
https://datazilla.mozilla.org/?start=1383322827&stop=1383927627&product=Firefox&repository=Mozilla-Inbound-Non-PGO&test=tsvgx&page=composite-scale-opacity.svg&project=talos
Updated•11 years ago
|
Blocks: 923193
Reporter | ||
Comment 7•11 years ago
|
||
on b2g-inbound and fx-team we seem to have fix the svg talos regression:
http://graphs.mozilla.org/graph.html#tests=[[281,64,31]]&sel=none&displayrange=30&datatype=running
I am not sure why I don't see this on the Firefox branch.
FWIW a number of bugs have been filed on SVG performance regressions blamed on bug 923193. At least some of them are also regressed by this bug. It's all a bit confusing, although I've tried to reduce the confusion by backing out 923193.
Reporter | ||
Comment 9•11 years ago
|
||
it appears we are back to normal, shall we close this or leave it open.
Please ask if you need any help reproducing this locally or working with talos.
Updated•11 years ago
|
Whiteboard: [talos_regression] → [c= p= s= u=1.3] [talos_regression]
Assignee | ||
Comment 10•11 years ago
|
||
(In reply to Joel Maher (:jmaher) from comment #9)
> it appears we are back to normal, shall we close this or leave it open.
If the Talos charts show us being close to back to where we were before this regression, that would most likely indicate that since this regression there have been perf gains in other areas that have canceled it out. (Or put another way, if correct, this regression is canceling out perf gains.) The code that caused the regression has still been in the tree, and I would doubt that it magically stopped being a perf drag.
There's no real need to keep the patch that caused this regression in the tree at the moment. Its goal was to remove some Thebes backed gfxContexts so that we can start removing a bunch of legacy code, but until far more/all instances of Thebes backed gfxContexts are removed it's not actually all that useful a change by itself. As discussed with Matt on IRC, I backed out the only part of the offending patch that could have caused the regression:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4ca041c97b4
Bug 934183 will re-remove the Thebes backed gfxContexts that the regressing patch removed, but it a way that shouldn't regress perf.
No longer depends on: 934183
Reporter | ||
Comment 11•11 years ago
|
||
this change looks to have improved the svg tests! Any objections to closing this bug?
Assignee | ||
Comment 12•11 years ago
|
||
Yes, it can just be closed when the commit is merged to mozilla-central as normal.
Comment 13•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Updated•11 years ago
|
Whiteboard: [c= p= s= u=1.3] [talos_regression] → [c=automation p= s=2013.12.06 u=1.3] [talos_regression]
Whiteboard: [c=automation p= s=2013.12.06 u=1.3] [talos_regression] → [c=automation p= s=2013.12.06 u=1.3] [talos_regression][qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•