Last Comment Bug 776747 - Talos Regression :( SVG, Opacity Row Major increase 4.54%
: Talos Regression :( SVG, Opacity Row Major increase 4.54%
Status: RESOLVED FIXED
[qa-]
: perf, regression
Product: Core
Classification: Components
Component: SVG (show other bugs)
: 17 Branch
: All All
: -- normal (vote)
: mozilla17
Assigned To: Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
:
Mentors:
Depends on:
Blocks: 614732
  Show dependency treegraph
 
Reported: 2012-07-23 16:18 PDT by Matt Brubeck (:mbrubeck)
Modified: 2012-10-16 16:30 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
patch (14.17 KB, patch)
2012-07-29 16:41 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
roc: review+
Details | Diff | Review

Description Matt Brubeck (:mbrubeck) 2012-07-23 16:18:49 PDT
It looks like this regression was probably caused by bug 614732:

Regression  SVG, Opacity Row Major increase 4.54% on MacOSX 10.7 Mozilla-Inbound
----------------------------------------------------------------------------------
    Previous: avg 96.900 stddev 0.563 of 30 runs up to revision a746aaa32b22
    New     : avg 101.300 stddev 0.570 of 5 runs since revision 20ba830b7dca
    Change  : +4.400 (4.54% / z=7.812)
    Graph   : http://mzl.la/MA0OrS

Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a746aaa32b22&tochange=20ba830b7dca
Comment 1 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-07-24 12:36:12 PDT
I can reproduce this running Talos locally with the following nightlies, but I'm not finding the output from the built-in profiler to be of much use in figuring out what's taking longer.

2012-07-20-03-05-49-mozilla-central
2012-07-21-03-05-55-mozilla-central

As far as builds I create locally go, I've not been able to reproduce it at all yet. I've tried creating opt builds with various config flags.
Comment 2 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-07-24 17:02:58 PDT
Current suspect is the last minute change in bug 614732 comment 79 to make nsSVGPathGeometryFrame::IsSVGTransformed return true if GetAnimatedTransformList() returns a non-null pointer without checking if the object pointed to has zero length. That means that we will _always_ return true from IsSVGTransformed, since nsSVGGraphicElement::GetAnimatedTransformList() always returns a non-null pointer. :-/

nsSVGDisplayContainerFrame::IsSVGTransformed and nsSVGForeignObjectFrame::IsSVGTransformed also don't check whether the list is empty.
Comment 3 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-07-25 08:16:44 PDT
I finally managed to coerce Try into giving me Talos results:

https://tbpl.mozilla.org/?tree=Try&rev=75b5f9108576

The IsSVGTransformed issue does indeed seem to be the problem, since that Try run actually resulted in a marginal pref improvement over the fastest result in the historic range used by compare.py.

There are two ways to fix this.

 1) Have the GetAnimatedTransformList implementations return nsnull if the
    'transform' attribute is not actually set.

 2) Have the IsSVGTransformed callers of GetAnimatedTransformList check the
    object that it returns to see if it has zero length.

Option 1 is complicated by the way that the DOM wrappers require an object to be returned. Option 2 is complicated by the fact that we need to reconstruct the frame tree when we switch from not-transformed to transformed.
Comment 4 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-07-29 16:41:57 PDT
Created attachment 647025 [details] [diff] [review]
patch

This patch makes GetAnimatedTransformList only allocate a gfxMatrix for elements when we need one. I.e. when:

 * the 'transform' attribute is set
 * the 'transform' attribute is animated
 * script is accessing the DOM wrapper (wrappers assume the internal
   object exists)
Comment 5 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-07-29 16:43:09 PDT
(In reply to Jonathan Watt [:jwatt] from comment #4)
> This patch makes GetAnimatedTransformList only allocate a gfxMatrix for

s/gfxMatrix/SVGAnimatedTransformList/
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-29 16:57:56 PDT
Comment on attachment 647025 [details] [diff] [review]
patch

Review of attachment 647025 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that.

::: content/svg/content/src/nsSVGElement.h
@@ +242,5 @@
>    }
>    // Despite the fact that animated transform lists are used for a variety of
>    // attributes, no SVG element uses more than one.
> +  virtual SVGAnimatedTransformList* GetAnimatedTransformList(
> +                                                     bool aAllocate = false) {

boolean parameters suck. Take a flags parameter and use an enum to define its values. E.g. enum { FORCE_ALLOCATE = 0x01 }, GetAnimatedTransformList(PRUint32). And document what it means!
Comment 7 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-07-29 17:36:54 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f964a99c62f0
Comment 8 Ed Morley [:emorley] 2012-07-30 02:44:07 PDT
https://hg.mozilla.org/mozilla-central/rev/f964a99c62f0
Comment 9 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-07-30 03:21:49 PDT
Comment 0 states that the previous average was 96.9, and http://mzl.la/MA0OrS shows the last four builds since the merge as averaging at 95.9, so it looks like this has more than fixed the perf regression.

Note You need to log in before you can comment on or make changes to this bug.