Closed
Bug 908067
Opened 11 years ago
Closed 11 years ago
Replace box-shadow fog effect with static images
Categories
(Firefox :: Theme, defect)
Tracking
()
RESOLVED
WONTFIX
People
(Reporter: MattN, Assigned: MattN)
References
Details
(Keywords: perf, Whiteboard: [Australis:P3][Australis:M?])
Attachments
(4 files, 1 obsolete file)
(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•11 years ago
|
||
(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•11 years ago
|
||
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
Updated•11 years ago
|
Whiteboard: [Australis:P?][Australis:M?]
Comment 3•11 years ago
|
||
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.
Whiteboard: [Australis:P?][Australis:M?]
Updated•11 years ago
|
Whiteboard: [Australis:P?][Australis:M?]
Comment 4•11 years ago
|
||
(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.
Assignee | ||
Comment 5•11 years ago
|
||
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?
Flags: needinfo?(tnikkel)
Comment 6•11 years ago
|
||
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.
Updated•11 years ago
|
Whiteboard: [Australis:P?][Australis:M?] → [Australis:P5][Australis:M?]
Comment 7•11 years ago
|
||
Upping the priority since this is apparently low-hanging fruit for our TART regression.
Whiteboard: [Australis:P5][Australis:M?] → [Australis:P1][Australis:M?]
Comment 8•11 years ago
|
||
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.
Summary: Australis box-shadow calculations are expensive on Windows 7 → Replace box-shadow fog effect with static images
Comment 9•11 years ago
|
||
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•11 years ago
|
||
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.
Flags: needinfo?(tnikkel)
Comment 11•11 years ago
|
||
I've filed the tabstrip repaint issue as bug 908796.
Comment 12•11 years ago
|
||
Assignee | ||
Comment 13•11 years ago
|
||
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
Attachment #795896 -
Flags: ui-review?(shorlander)
Attachment #795896 -
Flags: feedback?(mconley)
Assignee | ||
Comment 14•11 years ago
|
||
Assignee | ||
Comment 15•11 years ago
|
||
Assignee | ||
Comment 16•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
Lowering priority based on comment #17.
Whiteboard: [Australis:P1][Australis:M?] → [Australis:P3][Australis:M?]
Comment 20•11 years ago
|
||
Comment on attachment 795896 [details] [diff] [review]
WIP 1 - Using images from attachment 795467 [details]
Review of attachment 795896 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM.
Attachment #795896 -
Flags: feedback?(mconley) → feedback+
Comment 21•11 years ago
|
||
Comment on attachment 795896 [details] [diff] [review]
WIP 1 - Using images from attachment 795467 [details]
Clearing review until there is an updated patch.
Attachment #795896 -
Flags: ui-review?(shorlander)
Comment 22•11 years ago
|
||
Now that the Windows perf regression is under control this is no longer necessary.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•