Closed Bug 1035330 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla33

People

(Reporter: jmaher, Assigned: jwatt)

References

Details

(Keywords: perf, regression, Whiteboard: [talos_regression])

Attachments

(1 file)

patch
(deleted), patch
bas.schouten
: review+
Details | Diff | Splinter Review 
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.
and osx 10.8 and unbuntu 64.
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?
Flags: needinfo?(jwatt)
Which test file? Where do I find it?
Flags: needinfo?(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.
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
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?
Flags: needinfo?(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...
Flags: needinfo?(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...
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.
(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.
(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...
Attached patch patch (deleted) — — Splinter Review 
Let's try this.
Assignee: nobody → jwatt
Attachment #8453199 - Flags: review?(bas)
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 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?
Attachment #8453199 - Flags: review?(bas) → review+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/ba55afbced84
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
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.
Looks okay to me now. Joel?
Flags: needinfo?(jmaher)
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.
Flags: needinfo?(jmaher)
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: