908067 - Replace box-shadow fog effect with static images

Status: RESOLVED WONTFIX

(Firefox :: Theme, defect)

Description

[Matthew N. [:MattN]]

(Quoting Bas Schouten (:bas.schouten) from bug 902024 comment 10)
> We seem to be spending about twice as much time in
> ContainerState::ProcessDisplayItems, which seems to be a result of simply
> having more displayitems/layers. However, this only account for about 2% of
> the difference. We seem to be spending 10% vs 6% on EndTransaction which is
> the actual 'gfx part' of this. Some non-insignificant portion of that is
> spent doing background colors, probably a little bit more than in the m-c
> case because of having more displayitems (and possibly layers, but without a
> Moz2D recording or a layer tree dump it's hard to be sure), but by far the
> largest contribution is box shadow calculations (at ~3%). And that's also
> the main difference I find in the profiles for gfx related things. So
> shadows seem to be contributing most significantly to the difference.

The profile in question was on Windows 7 with Aero Glass. This would likely point to the full-width box-shadow for the Aero Glass fog behind the tabstrip being a large portion of the box-shadow cost.

I'll do a quick test of the cost of commenting out the glass fog to see how much of an improvement it gives. We can figure out how to make this faster afterwards.

Comment 1

[Dão Gottwald [:dao]]

(In reply to Matthew N. [:MattN] from comment #0)
> (Quoting Bas Schouten (:bas.schouten) from bug 902024 comment 10)
> > largest contribution is box shadow calculations (at ~3%). And that's also
> > the main difference I find in the profiles for gfx related things. So
> > shadows seem to be contributing most significantly to the difference.
> 
> The profile in question was on Windows 7 with Aero Glass. This would likely
> point to the full-width box-shadow for the Aero Glass fog behind the
> tabstrip being a large portion of the box-shadow cost.

I thought box-shadow was somehow cached these days such that it doesn't need to be recalculated all the time.

I just enabled paint flashing in an UX build and don't even see the shadow area being repainted when opening a tab, but I do see it when closing a tab while tabs aren't overflowing -- why, I don't know.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Dropping the box-shadow from the fog showed some pretty significant results on my Windows 7 machine.

For 25 runs of "simple":


Local UX - base

TART.simple-open-DPI1.half    Average (25): 2.50 stddev: 0.09
TART.simple-open-DPI1.all    Average (25): 2.89 stddev: 0.08
TART.simple-open-DPI1.error    Average (25): 6.37 stddev: 1.45

TART.simple-close-DPI1.half    Average (25): 2.46 stddev: 0.84
TART.simple-close-DPI1.all    Average (25): 3.00 stddev: 0.77
TART.simple-close-DPI1.error    Average (25): 19.28 stddev: 0.41


Local UX - no box shadow on fog

TART.simple-open-DPI1.half    Average (25): 1.95 stddev: 0.04
TART.simple-open-DPI1.all    Average (25): 2.32 stddev: 0.05
TART.simple-open-DPI1.error    Average (25): 4.14 stddev: 1.07

TART.simple-close-DPI1.half    Average (25): 1.54 stddev: 0.14
TART.simple-close-DPI1.all    Average (25): 2.06 stddev: 0.13
TART.simple-close-DPI1.error    Average (25): 19.81 stddev: 2.29

Comment 3

[Mike Conley (:mconley) (:⚙️)]

shorlander has offered up the possibility of using static bitmaps instead of the box-shadow to create the fog effect. We say the word and he'll cut it up.

Comment 4

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) from comment #3)
> shorlander has offered up the possibility of using static bitmaps instead of
> the box-shadow to create the fog effect. We say the word and he'll cut it up.

We should still figure out why this area is being redrawn in some situations (see comment 1) and maybe file a platform bug on box-shadow being expensive here.

Comment 5

[Matthew N. [:MattN]]

Ideally the box-shadow would be cached in layout/gfx (if it's not already) like we do for some gradients. tnikkel, do you know whether this is already the case and whether this is something that would be possible?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

According to jrmuizel, we do not cache box-shadows - and while that's something that we might do in the future, it would not be wise to block Australis on it (he indicated that this might be quite a bit of work to implement).

He says that the shortest path to success is almost certainly using the images as shorlander had suggested.

I do agree that platform bugs should be filed for these things to speed them up - but I don't think they can block Australis.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Upping the priority since this is apparently low-hanging fruit for our TART regression.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Looks like we're going with some images here. I'll ask BenWa what's going on with the shadow being re-painted on tab close, and if needs be, open a new bug on that.

Comment 9

[Dão Gottwald [:dao]]

The shadow area being re-painted at all in that tab close case might well be a front-end bug, whereas box-shadow being slow would be a platform bug.

Comment 10

[Timothy Nikkel (:tnikkel)]

Looks like the questions have mostly been answered already. But yes we should see why the box shadow is getting repainted at all. There are already platform bugs on box shadow being slow, if you don't find one you like please provide a testcase.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

I've filed the tabstrip repaint issue as bug 908796.

Comment 12

[Stephen Horlander [:shorlander]]

Attachment: Background Fog Images - i01

Comment 13

[Matthew N. [:MattN]]

Attachment: WIP 1 - Using images from attachment 795467

This works but isn't as good as the existing box-shadow IMO because the top, left, and right edges have hard edges of the fog (i.e. they don't transition to an opacity of 0). 
The left and right sides matter since the window frame is not all XUL so you can see the edge against the native frame in a restored window. 
The top matters when you enter customization mode and the padding of the mode either makes the top of the image visible if we continue to anchor the fog to the tabs (as I think we should) or the tab text appears in the faint bottom part of the fog which somewhat defeats the point of the fog.

Stephen, does this make sense? Can we have new images where the top, left and right edges of the fog transitioning to an opacity of 0?

Perf baseline: https://tbpl.mozilla.org/?tree=Try&rev=9d1bfc6c2823
Try push: https://tbpl.mozilla.org/?tree=Try&rev=c994f70fe39f

Comment 14

[Matthew N. [:MattN]]

Attachment: Screenshot of restored window showing clipping on left and right against window frame

Comment 15

[Matthew N. [:MattN]]

Attachment: Screenshot of customization mode showing clipping on the top of the image

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 795896 [details] [diff] [review]
WIP 1 - Using images from attachment 795467 [details]

This approach seemed to give ~12% win on TART for Windows 7 (which is the only affected platform on talos).
http://compare-talos.mattn.ca/index.html?oldRevs=9d1bfc6c2823&newRev=c994f70fe39f&submit=true

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Hm. Right, so this only affect Windows 7...

According to http://compare-talos.mattn.ca/?oldRevs=dc7b76fcf7e4&newRev=d7ac6fe877d2&server=graphs.mozilla.org&submit=true (more specifically, http://compare-talos.mattn.ca/breakdown.html?oldTestIds=28914079,28916563,28916571,28916591,28916601,28916643,28916699,28916715,28916725,28916745,28916757,28916765,28916775,28916861,28916903,28924069&newTestIds=28858333,28862989,28863033,28863081&testName=tart&osName=Windows%207&server=graphs.mozilla.org), we appear to be kicking all sorts of ass on Windows 7 on the talos hardware.

I wonder if we should shelve this idea temporarily until we can get XP (which is the major platform that's really hurting on the talos hardware - http://compare-talos.mattn.ca/breakdown.html?oldTestIds=28914099,28916763,28916771,28916821,28916829,28916835,28924115&newTestIds=28858467,28863131,28863137,28863147&testName=tart&osName=Windows%20XP&server=graphs.mozilla.org) whipped into shape.

If, after XP (and I suppose OS X and Ubuntu) are confirmed to be neutral on the talos hardware, we might want to come back to this for the weaker hardware (Avi's AMD laptop, which is running Windows 7).

So, my recommendation is to hold on this idea for now until we have XP whipped into shape and we have more data from Avi's machine.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Lowering priority based on comment #17.

Comment 19

[Stephen Horlander [:shorlander]]

Attachment: Background Fog Images - i02

Better images

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 795896 [details] [diff] [review]
WIP 1 - Using images from attachment 795467 [details]

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

LGTM.

Comment 21

[Stephen Horlander [:shorlander]]

Comment on attachment 795896 [details] [diff] [review]
WIP 1 - Using images from attachment 795467 [details]

Clearing review until there is an updated patch.

Comment 22

[Jared Wein [:jaws] (please needinfo? me)]

Now that the Windows perf regression is under control this is no longer necessary.