Closed Bug 935008 Opened 11 years ago Closed 10 years ago

SVG Talos tests have regressed due to bug 922942

Categories

(Core :: Graphics, defect)

28 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28
blocking-b2g 1.3+
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.
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
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
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)
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)
(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+
Whiteboard: [talos_regression]
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.
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.
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.
Whiteboard: [talos_regression] → [c= p= s= u=1.3] [talos_regression]
(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
this change looks to have improved the svg tests!  Any objections to closing this bug?
Yes, it can just be closed when the commit is merged to mozilla-central as normal.
https://hg.mozilla.org/mozilla-central/rev/d4ca041c97b4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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.