GNOME Bugzilla – Bug 675111
frames: Remove frame pixel caching
Last modified: 2012-05-25 17:16:20 UTC
When I was writing the CSS frame decorations code, I was getting a bit fed up with the caching stuff. So I decided to remove it, as an experiment, just to see how much slower... It turned out to be faster. I'm going to guess that it's because we're now only calling meta_frames_paint once instead of four times to cache the pixels, plus N more times to cover whatever else the cache didn't cover. A smarter person might be able to paint one, shatter the surfaces into subsurfaces to only paint once, but whatever.
Created attachment 213072 [details] [review] frames: Remove frame pixel caching Caching the frame pixels actually slows us down and increases our memory footprint, as we have to render the frame more than once.
Created attachment 213073 [details] [review] frames: Remove frame pixel caching Caching the frame pixels actually slows us down and increases our memory footprint, as we have to render the frame more than once. Fix build (go rebase!) and remove the now unnecessary invalidate_frames GList.
OK, so some numbers. It doesn't really matter too much, but from very basic tests, it *is* faster. Around 0.8-0.9ms with frame pixel caching. Around 0.4-0.5ms without.
Review of attachment 213073 [details] [review]: It looks to me like the caching got broken a bit with the introduction of invisible borders - if I add a print to the end section of meta_frames_draw() then every time a frame with a invisible border gets resized, I see rectangles like the following drawn: UC: 0 0 805 8! UC: 0 8 9 759! UC: 796 8 9 759! UC: 0 767 805 9! So I'm not sure I trust the timing you did - since we're basically hitting all the painting logic twice even if not much actually gets painted in the invisible borders. But certainly as long as we're only drawing frames on redirected windows there's no point to having this caching infrastructure - we're caching them in the server permanently, so we don't need some fancy drop-on-idle cache in the client. (If we ever went to a compiz-style approach where we drew the decorations in the compositor directly instead of drawing them onto the window borders, then we'd need something like this, but that doesn't seem to be the direction we're going. We can always resurrect or rewrite if necessary) ::: src/ui/frames.c @@ +1911,1 @@ clip_to_screen (region, frame); I'm not sure how this is working - if we map a window partially offscreen, we should still get exposes on the offscreen part when the window is redirected. If we ignore those exposes, we won't get more when the window is dragged onscreen, so won't we be missing the new areas? This used to be after cached_pixels_draw() so we'd only have problems more than twice the screen width or height, but now it looks like it would affect any window. @@ +1911,2 @@ clip_to_screen (region, frame); subtract_client_area (region, frame); I think you should also clip the invisible borders out of the picture - it doesn't matter a lot - the theme shouldn't be drawing anything inside them, but I could see it simplifying the region to draw or making it much smaller in some cases - like if we aren't drawing any visible border at all on some sides of the window but still have invisible borders. @@ +1912,3 @@ subtract_client_area (region, frame); + if (cairo_region_num_rectangles (region) == 0) more idiomatic to write cairo_region_is_empty (region) @@ -2166,3 @@ - n_areas = cairo_region_num_rectangles (region); - - for (i = 0; i < n_areas; i++) I wouldn't be shocked if it was more efficient to keep this break-into-rectangles approach - for a typical frame shape, if something in the drawing code does cairo_push_group() <...> cairo_pop_group_to_source() then the group pixmap could be the size of the window instead of just a few pixels big. But let's keep it simple for now and hope that having everything clipped down to what we need drawn will avoid any expensive intermediate operations. @@ +1922,2 @@ + gdk_cairo_region (cr, region); + cairo_clip (cr); You should do this clip before ou draw the background pattern above
Created attachment 214570 [details] [review] frames: Remove frame border pixel caching and related optimizations Since we now cache windows in the X server, we don't really need to cache them here. Since we are redirecting windows in most cases, we're not gaining anything except added memory usage. Additionally, remove the clip to screen optimization - if a window is partially off-screen, we still need to draw the entire thing as redirection means we won't get an expose event for it. Additionally, when introducing invisible borders, something accidentally slipped through: we were getting expose events on the invisible borders, and they weren't in the cached pixels rect, so we were painting the theme for them, even if we didn't actually paint anything with cairo. Make sure to clip out the invisible borders instead of just the client rect so that we don't draw if our expose event is on the invisible borders.
(In reply to comment #4) > Review of attachment 213073 [details] [review]: > > ::: src/ui/frames.c > @@ +1911,1 @@ > clip_to_screen (region, frame); > > I'm not sure how this is working It wasn't. Thanks for noticing! > @@ +1911,2 @@ > clip_to_screen (region, frame); > subtract_client_area (region, frame); > > I think you should also clip the invisible borders out of the picture - it > doesn't matter a lot - the theme shouldn't be drawing anything inside them, but > I could see it simplifying the region to draw or making it much smaller in some > cases - like if we aren't drawing any visible border at all on some sides of > the window but still have invisible borders. This is an easy win. Done. > @@ +1912,3 @@ > subtract_client_area (region, frame); > > + if (cairo_region_num_rectangles (region) == 0) > > more idiomatic to write cairo_region_is_empty (region) Aha, thanks! > @@ -2166,3 @@ > - n_areas = cairo_region_num_rectangles (region); > - > - for (i = 0; i < n_areas; i++) > > I wouldn't be shocked if it was more efficient to keep this > break-into-rectangles approach - for a typical frame shape, if something in the > drawing code does > > cairo_push_group() > <...> > cairo_pop_group_to_source() > > then the group pixmap could be the size of the window instead of just a few > pixels big. But let's keep it simple for now and hope that having everything > clipped down to what we need drawn will avoid any expensive intermediate > operations. We're going to be doing that sort of thing in a future patch to work around a cairo bug and I'm quite sure that GTK+ does something similar internally. We can always re-add it if it turns out to be a big memory win. > @@ +1922,2 @@ > + gdk_cairo_region (cr, region); > + cairo_clip (cr); > > You should do this clip before ou draw the background pattern above OK.
Review of attachment 214570 [details] [review]: Looks good
Attachment 214570 [details] pushed as 6fb857c - frames: Remove frame border pixel caching and related optimizations