1035330 - 7.5% Win7/8/XP svg opacity regression July 6th (fx33) on inbound from rev e91a7e542b7b

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

Here is a graph showing the regression:
http://graphs.mozilla.org/graph.html#tests=[[225,131,25]]&sel=1404576771000,1404749571000&displayrange=7&datatype=running

here is the changeset:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e91a7e542b7b

here is a link to tbpl with some retriggers to show the difference:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&fromchange=60d0ee6ffa30&tochange=3a090599c39e&jobname=Windows%207%2032-bit%20mozilla-inbound%20talos%20svgr

here is a link to some documentation about tsvg-opacity:
https://wiki.mozilla.org/Buildbot/Talos/Tests#tsvg-opacity

Prior to the patch we would get ~260 and after the patch we get ~280 for returned values for windows 7.

This should be easy to test your fix on try server.

Comment 1

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

and osx 10.8 and unbuntu 64.

Comment 2

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

results for pgo are coming in:
http://54.215.155.53:8080/alerts.html?rev=ceff7d54080f&showall=1&testIndex=0&platIndex=0
winxp: 4.24%
win7:  7.33%
win8:  6.02%

pgo regressions are the ones that end up getting uplifted.

JWatt, can you look into this?

Comment 3

[Jonathan Watt [:jwatt]]

Which test file? Where do I find it?

Comment 4

[Jonathan Watt [:jwatt]]

Running the tests in:

http://hg.mozilla.org/build/talos/raw-file/tip/talos/page_load_test/svg_opacity/

under Instruments, less than 0.4% of the time is spent under GetBBoxContribution, so I don't understand this regression.

Comment 5

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

prior changeset (https://tbpl.mozilla.org/php/getParsedLog.php?id=43255447&tree=Mozilla-Inbound&full=1):
08:41:15     INFO -  |0;big-optimizable-group-opacity-2500.svg;545;268;255;262;266;257;257;266;265;266;261;254;259;263;259;262;260;266;266;263;256;263;261;262;253
08:41:15     INFO -  |1;small-group-opacity-2500.svg;801;852;840;852;854;852;853;854;853;858;842;854;856;847;841;844;848;856;852;853;849;852;838;844;854

offending changeset (https://tbpl.mozilla.org/php/getParsedLog.php?id=43261140&tree=Mozilla-Inbound&full=1):
10:04:57     INFO -  |0;big-optimizable-group-opacity-2500.svg;566;279;277;277;289;278;272;273;275;273;273;276;273;276;276;275;279;278;278;277;274;273;273;280;274
10:04:57     INFO -  |1;small-group-opacity-2500.svg;835;896;857;866;885;885;892;899;868;891;882;880;887;887;883;862;866;891;872;880;878;857;857;879;890


graph server ignored the highest value, so we are only testing big-optimizable-group-opacity-2500.svg and the raw values there show an increase.

This datazilla graph (assuming it loads) will help show the difference as well:
https://datazilla.mozilla.org/?start=1404143625&stop=1404748425&product=Firefox&repository=Mozilla-Inbound-Non-PGO&os=win&os_version=6.1.7601&test=tsvgr_opacity&graph_search=5f27f634ed58&x86_64=false&project=talos

Comment 6

[Bas Schouten (:bas.schouten)]

My guess here for the cause is that before, you were just emitting a 'rect' instruction somewhere, gfxContext translated that to a FillRect call. Now you might be using the full power of a path there because there's no way of knowing your path contains only a single rect.

That's just a guess though. What do you think?

Comment 7

[Jonathan Watt [:jwatt]]

Joel: thanks for the links.

Bas: I was about to say "no", since all we've done is change what underlies the gfxContext without any changes to the calls we make to the gfxContext. But then I thought I'd better check what gfxContext does under the hood and, what do you know, it does indeed use a path builder in gfxContext::Rectangle()! But then I put in some breakpoints and we never actually hit that code since |!mPathBuilder && !mPathIsRect| always evaluates to true...so I was about to say "no" again. Then I realized that right after the GeneratePath() call in nsSVGPathGeometryFrame::GetBBoxContribution we call tmpCtx->IdentityMatrix(), and gfxContext::IdentityMatrix() calls gfxContext::ChangeTransform() which turns the rect into a Path object...pfft...

Comment 8

[Jonathan Watt [:jwatt]]

Oh, wait. gfxContext::ChangeTransform() only turns the rect into a Path object if |toNewUS.IsRectilinear()| is false, which it never is for these talos tests. So in fact we are not creating a Path object...

Comment 9

[Jonathan Watt [:jwatt]]

Right now the Gecko Profiler is not producing stacks for these tests for some reason, and Instruments shows less than 0.5% of the time being spent under GetBBoxContribution(), so for now I've no data on where the extra time is coming from.

Comment 10

[Avi Halachmi (:avih)]

(In reply to Jonathan Watt [:jwatt] from comment #9)
> Right now the Gecko Profiler is not producing stacks for these tests for
> some reason, and Instruments shows less than 0.5% of the time being spent
> under GetBBoxContribution(), so for now I've no data on where the extra time
> is coming from.

Were you able to reproduce the regression with and without the patch? if it manifests on your system, then grabbing the data should be possible. but if you can't reproduce, then we need to understand why.

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #8)
> Oh, wait. gfxContext::ChangeTransform()...

Oh, wait. It's later still when calling gfxContext::GetUserPathExtent() that the rect is changed to a path. Ho-hum...

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: patch

Let's try this.

Comment 13

[Avi Halachmi (:avih)]

It might be useful to push to try two builds: current and current + new patch, and compare the numbers which tsvgr_opacity produces to test if the patch helps, instead of pushing the patch and then maybe pushing another patch etc.

Comment 14

[Bas Schouten (:bas.schouten)]

Comment on attachment 8453199 [details] [diff] [review]
patch

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

Don't you need to check if the transform is rectilinear?

Comment 15

[Jonathan Watt [:jwatt]]

(In reply to Bas Schouten (:bas.schouten) from comment #14)
> Don't you need to check if the transform is rectilinear?

Nope. That would be the case if we were drawing the rect and wanted to draw it without converting it to a Path. But here we're just getting the bounds of the transformed rect, which TransformBounds() will do just fine.

Comment 16

[Jonathan Watt [:jwatt]]

(In reply to Avi Halachmi (:avih) from comment #13)
> It might be useful to push to try two builds: current and current + new
> patch, and compare the numbers which tsvgr_opacity produces to test if the
> patch helps, instead of pushing the patch and then maybe pushing another
> patch etc.

This is a good patch to have, even if it didn't fix these regressions.

Comment 17

[Jonathan Watt [:jwatt]]

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

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

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

I have limited data here- this definitely resulted in a performance improvement, it is hard to tell if it fixes the entire regression- pgo data will be the real test.

Comment 20

[Jonathan Watt [:jwatt]]

Looks okay to me now. Joel?

Comment 21

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

all is well (osx seems to be a net win), except for win8:
http://graphs.mozilla.org/graph.html#tests=[[225,131,31],[225,63,31]]&sel=1402506357518,1405098357518&displayrange=30&datatype=running

win8 has the large majority fixed, but you can see by the graphs that it is still showing a slight regression for both pgo and non-pgo.

Comment 22

[Avi Halachmi (:avih)]

(In reply to Joel Maher (:jmaher) from comment #21)
> all is well (osx seems to be a net win), except for win8:
> http://graphs.mozilla.org/graph.html#tests=[[225,131,31],[225,63,
> 31]]&sel=1402506357518,1405098357518&displayrange=30&datatype=running
> 
> win8 has the large majority fixed, but you can see by the graphs that it is
> still showing a slight regression for both pgo and non-pgo.

Just to express it here such that others don't have to look at the graph:

Windows 8, PGO:

Until July-6th: 233 ms
July 6th-10th (this regression): 247 ms.
July 10th onwards (the fix): 236 ms

So overall, from before the regression until after the fix, win8 regressed about 1%. I think we can live with this very well. Let's put this bug behind us.