GNOME Bugzilla – Bug 697274
Unlock animation is unusably slow
Last modified: 2013-05-03 19:57:10 UTC
Hi there, on my Intel (Sandy Brige generation) i7, gnome-shell 3.8's unlock animation renders at about one frame every five seconds. It always makes me wonder whether the machine has crashed, or is about to. (3.6 was fine, at--I'd guess--tens of frames a second.) If that matters, I am using a dual-screen setup with my laptop and a 1080p LCD side-by-side. See the Debian bug template for software versions. The intel driver is at 2.20.14-1. Andreas
Created attachment 240615 [details] Debian-generated software info
I'm able to reproduce this as well, but only on dual head.
<drago01> Jasper: seems to work much better with the patch from https://bugzilla.gnome.org/show_bug.cgi?id=689858 <Services> Bug 689858: normal, Normal, ---, gnome-shell-maint, UNCONFIRMED, St: optimize box-shadow rendering <drago01> Jasper: seems like creating two box shadows for such large textures is a bit too much <drago01> Jasper: we do spend 26% of the time in blur_pixels
Some more notes: 1) This isn't SNB specific happens on ironlake and gm45 as well 2) Removing box shadow seems to fix it (hence why I tried with the optimize box shadow patch).
This is caused by the interning of theme nodes -- as theme nodes also cache the paint state along with their last allocation, if two actors have the same theme node but different sizes, they'll get in a battle repainting and overwriting the last painted actor. A patch stack is attached here to let individual widgets cache the paint state, but if this is considered too intrusive for 3.8.1, we can simply turn off theme node interning for the gnome-3-8 branch and put this on master.
Created attachment 240677 [details] [review] st-theme-node-drawing: Depend less on the paint state in some helper functions We want to put the paint state in the actor rather than in the theme node, as having two actors with different sizes but the same theme node is now much less efficient.
Created attachment 240678 [details] [review] st-theme-node-drawing: Move most of the cached paint state to another struct Since we now share theme nodes between, we shouldn't cache the paint state across all nodes. As a first step towards putting this in the actor, split out the state into another structure. Keep it in the theme node for now so that we don't make too many changes in one commit. It's possible that some of these pieces of drawing state could be shared between theme nodes. For the sake of simplicity, assume that none of them are shared or should be shared. A future commit could identify those that could be shared and move them back into the theme node.
Created attachment 240679 [details] [review] st: Move the theme node drawing state to StWidget This ensures that two widgets sharing the same theme node won't trample on each other's prerendered materials if the actors are of different sizes. This also tries to be very careful to share as much as possible during a transition. This has the side effect that if a widget changes state a bunch of times, we won't cache every state. Since we expect that state changes are infrequent and that most cases we'll be able to use the texture cache to do most of the heavy lifting, this cost is much more insignificant than rendering a number of different actors with the same theme node and different sizes.
Created attachment 240680 [details] [review] st-theme-node: Move some allocation-independent textures back to the theme node The background image, background image shadow and border image are allocation-indepedent, so we can keep these in the node. Given that these are are likely cached in the StTextureCache, the slight increase in code complexity may not be worth caching these textures and materials -- we might be better off just computing when we need to paint.
Created attachment 240683 [details] [review] st-theme-node: Move some allocation-independent textures back to the theme node The background image, background image shadow and border image are allocation-indepedent, so we can keep these in the node. Given that these are are likely cached in the StTextureCache, the slight increase in code complexity may not be worth caching these textures and materials -- we might be better off just computing when we need to paint. Properly invalidate non-prerendered background / border images when their file monitor fires.
Created attachment 240684 [details] [review] st-widget: Cache two paint states In most cases, we'll transition between two states on hover / focus. Instead of recalculating and repainting our resources on state change, simply cache the last state when we transition. This probably isn't worth the effort, but I wrote it just in case somebody might want to investigate later. Also, it was fun.
Created attachment 240685 [details] [review] st-theme-node: Add a to_string function for debugging purposes Not crucial -- for debugging purposes.
Created attachment 240686 [details] [review] st-theme-node-drawing: Print a message on repaints for size changes For debugging purposes only.
Created attachment 240690 [details] [review] st-theme-node: Move some allocation-independent textures back to the theme node The background image, background image shadow and border image are allocation-indepedent, so we can keep these in the node. Given that these are are likely cached in the StTextureCache, the slight increase in code complexity may not be worth caching these textures and materials -- we might be better off just computing when we need to paint. Fix prerendered nodes with background images, like the avatar widget in the user menu.
Created attachment 240782 [details] [review] st-theme-context: Disable interning of nodes for now An alternate patch for the GNOME 3.8.1 release if we consider the above patch stack to be too invasive.
(In reply to comment #15) > An alternate patch for the GNOME 3.8.1 release if we consider the above patch > stack to be too invasive. The least intrusive change IMHO would be to disable the box-shadow in question.
(In reply to comment #16) > (In reply to comment #15) > > An alternate patch for the GNOME 3.8.1 release if we consider the above patch > > stack to be too invasive. > > The least intrusive change IMHO would be to disable the box-shadow in question. Yeah it is not *that* noticeable anyway. We should fix it properly for 3.10 (the patches from here + optimization patch).
Created attachment 240783 [details] [review] screenshield: Remove box-shadow This causes very low performance in some situations (like multiscreen). Proper fixes are too invasive at this point (3.8.1) so lets just remove the shadow for now and add it back later once he have fixed it.
Review of attachment 240783 [details] [review]: Let's go with this for now.
Review of attachment 240782 [details] [review]: Nope
Comment on attachment 240783 [details] [review] screenshield: Remove box-shadow Attachment 240783 [details] pushed as 98e74df - screenshield: Remove box-shadow
*** Bug 697633 has been marked as a duplicate of this bug. ***
Review of attachment 240677 [details] [review]: OK.
Can you rebase the others?
Created attachment 243191 [details] [review] st-theme-node-drawing: Depend less on the paint state in some helper functions We want to put the paint state in the actor rather than in the theme node, as having two actors with different sizes but the same theme node is now much less efficient.
Created attachment 243192 [details] [review] st-theme-node-drawing: Move most of the cached paint state to another struct Since we now share theme nodes between, we shouldn't cache the paint state across all nodes. As a first step towards putting this in the actor, split out the state into another structure. Keep it in the theme node for now so that we don't make too many changes in one commit. It's possible that some of these pieces of drawing state could be shared between theme nodes. For the sake of simplicity, assume that none of them are shared or should be shared. A future commit could identify those that could be shared and move them back into the theme node.
Created attachment 243193 [details] [review] st: Move the theme node drawing state to StWidget This ensures that two widgets sharing the same theme node won't trample on each other's prerendered materials if the actors are of different sizes. This also tries to be very careful to share as much as possible during a transition. This has the side effect that if a widget changes state a bunch of times, we won't cache every state. Since we expect that state changes are infrequent and that most cases we'll be able to use the texture cache to do most of the heavy lifting, this cost is much more insignificant than rendering a number of different actors with the same theme node and different sizes.
Created attachment 243194 [details] [review] st-theme-node: Move some allocation-independent textures back to the theme node The background image, background image shadow and border image are allocation-indepedent, so we can keep these in the node. Given that these are are likely cached in the StTextureCache, the slight increase in code complexity may not be worth caching these textures and materials -- we might be better off just computing when we need to paint.
Created attachment 243195 [details] [review] Revert "screenshield: Remove box-shadow" This reverts commit 98e74dfd38bd44154e002000ed46be4ed622173d.
Created attachment 243196 [details] [review] st-widget: Cache two paint states In most cases, we'll transition between two states on hover / focus. Instead of recalculating and repainting our resources on state change, simply cache the last state when we transition.
Created attachment 243197 [details] [review] st-theme-node: Add a to_string function for debugging purposes
Review of attachment 240686 [details] [review]: No.
Review of attachment 243191 [details] [review]: OK.
Review of attachment 243192 [details] [review]: LG.
Review of attachment 243193 [details] [review]: LG.
Review of attachment 243194 [details] [review]: Makes sense, code looks good.
Review of attachment 243195 [details] [review]: Performance is indeed restored with this patches so we can add the shadow back.
Review of attachment 243196 [details] [review]: ::: src/st/st-widget.c @@ +273,3 @@ } +static inline void Just trust the compiler unless you have a reason to think that it will get it wrong. @@ +279,3 @@ +} + +static inline StThemeNodePaintState * Same here.
Review of attachment 243197 [details] [review]: OK.
Attachment 243191 [details] pushed as 40b895d - st-theme-node-drawing: Depend less on the paint state in some helper functions Attachment 243192 [details] pushed as 72bc46d - st-theme-node-drawing: Move most of the cached paint state to another struct Attachment 243193 [details] pushed as 49064ed - st: Move the theme node drawing state to StWidget Attachment 243194 [details] pushed as c29aaa0 - st-theme-node: Move some allocation-independent textures back to the theme node Attachment 243195 [details] pushed as db14dc9 - Revert "screenshield: Remove box-shadow" Attachment 243196 [details] pushed as a2fcbb7 - st-widget: Cache two paint states Attachment 243197 [details] pushed as 53c609c - st-theme-node: Add a to_string function for debugging purposes Pushed with suggested changes.