GNOME Bugzilla – Bug 765887
"Background" / "Lock Screen" buttons losing / gaining focus (oddly) is CPU intensive
Last modified: 2018-01-04 15:30:30 UTC
Within the G-C-C backgrounds panel, when the buttons for "Background" and "Lock Screen" gain or lose focus, as when moving the mouse around the window or the window losing / gaining focus, is oddly CPU intensive. Looking at the output of sysprof when using the background panel a bit, it seems the image thumbnails are recalculated _every_ _time_ the window focus changes / the mouse cursor enters / leaves the button. That does not seem very smart ... Calculating the thumbnails ONCE when entering the backgrounds panel / when changing one of the images seems more appropriate. Or even calculating them ONCE for every setting change and saving the thumbnail to disk - e.g. to G-C-C cache directory. This is part of the sysprof output after moving the mouse around the backgrounds panel (not clicking anything) or having the G-C-C window lose / gain focus (doing this for ~1 min creates more than 100000 samples): Note the amount of calls to libjpeg / libpng. SELF CUMULATIVE FUNCTION [ 0.00%] [ 100.00%] [gnome-control-center] [ 27.07%] [ 27.13%] inflate_fast [ 25.05%] [ 25.11%] png_read_filter_row_paeth_multibyte_pixel [ 14.55%] [ 14.60%] scale_line [ 0.00%] [ 11.71%] In file [heap] [ 3.91%] [ 3.92%] decode_mcu [ 0.01%] [ 3.01%] sep_upsample [ 2.79%] [ 2.79%] adler32 [ 2.76%] [ 2.77%] inflate [ 1.75%] [ 1.75%] crc32 [ 0.91%] [ 0.91%] inflate_table [ 0.01%] [ 0.81%] In file /usr/lib64/libpthread-2.23.so [ 0.01%] [ 0.57%] In file /usr/lib64/libc-2.23.so [ 0.54%] [ 0.54%] png_read_filter_row_avg [ 0.50%] [ 0.50%] __memcpy_avx_unaligned [ 0.00%] [ 0.29%] gtk_css_value_image_compute [ 0.20%] [ 0.20%] gdk_cairo_surface_paint_pixbuf [ 0.18%] [ 0.18%] __GI_memcpy [ 0.14%] [ 0.14%] _gdk_pixbuf_get_module [ 0.13%] [ 0.13%] pixops_process [ 0.11%] [ 0.11%] png_read_filter_row_up [ 0.11%] [ 0.11%] gdk_pixbuf_fill [ 0.10%] [ 0.10%] process_pixel
Created attachment 359916 [details] [review] Patch Fixed by caching pixbuf instances in CcBackgroundPanelPrivate. Now cpu usage spikes to only 11% on my machine (compared to 100% before), animation is smooth now.
Created attachment 359932 [details] [review] Patch updated Cleaned up unneeded TODO comment
Review of attachment 359932 [details] [review]: Thanks for the patch. Can you update it to follow the coding style of the existing code? The commit message should start with the panel name like "background: ..." ::: panels/background/cc-background-panel.c @@ +51,3 @@ +enum background_type { WORKSPACE, LOCK }; +typedef enum background_type background_type_t; types are CamelCase, but I don't think we need these @@ +65,3 @@ CcBackgroundItem *current_background; CcBackgroundItem *current_lock_background; + spurious white space change @@ +76,3 @@ + + GdkPixbuf *current_workspace_pixbuf; + GdkPixbuf *current_lock_pixbuf; since these are tightly coupled with the CcBackgroundItem instances I'd prefer that we cache them there with g_object_set_data_full() which will take care of automatic free'ing them when needed as well @@ +84,3 @@ +#define CURRENT_BACKGROUND(type) (type_of_background == WORKSPACE ? priv->current_background : priv->current_lock_background) +#define CURRENT_PIXBUF(type) (type_of_background == WORKSPACE ? priv->current_workspace_pixbuf : priv->current_lock_pixbuf) I realize we already use this kind of macro above but I personally don't like it and since they're used only once, please inline it there. this also means the enum isn't that useful so I don't think we need it @@ +181,3 @@ priv->current_lock_background = current_background; + g_object_unref (priv->current_lock_pixbuf); + priv->current_lock_pixbuf = NULL; see the comment about g_object_set_data() @@ +222,3 @@ update_display_preview (CcBackgroundPanel *panel, GtkWidget *widget, + background_type_t type_of_background) the enum isn't that useful @@ +248,3 @@ cairo_paint (cr); g_object_unref (pixbuf); + } else { please follow the existing code indentation @@ +253,3 @@ + + if (!item) { + return; style for single line conditions is to not use braces anyway this isn't needed and in fact we would leak the cairo context if we returned here @@ +551,3 @@ priv->current_lock_background = configured; + g_object_unref (priv->current_lock_pixbuf); + priv->current_lock_pixbuf = NULL; same comment as above about g_object_set_data()
Thanks Rui, everything is addressed.
Created attachment 360207 [details] [review] Patch updated
Review of attachment 360207 [details] [review]: Thanks, I think we can still improve it a bit ::: panels/background/cc-background-panel.c @@ +223,2 @@ if (current_background == priv->current_background && panel->priv->display_screenshot != NULL) we should cache the resized screenshot as "pixbuf" on the background item as well, see below @@ +238,3 @@ + { + item = current_background == priv->current_background ? priv->current_background : priv->current_lock_background; + pixbuf = g_object_get_data(item, "pixbuf"); style is to leave space between function name and opening parenthesis @@ +242,3 @@ + gtk_widget_get_allocation (widget, &allocation); + + if (pixbuf == NULL) { style is: is (condition) { statement; } @@ +257,3 @@ + pixbuf, + 0, 0); + cairo_paint (cr); I think this coud would be more elegant if we split it into 2 steps. First, getting the pixbuf or creating and caching it appropriately according to the background item and existence of screenshot. Secondly the actual drawing/cairo paint at the end.
Yeah, good suggestion. Updated
Created attachment 360212 [details] [review] Patch updated
Review of attachment 360212 [details] [review]: there's still a few nits pointed below and mis-indentations but I'll amend the patch locally and push, thanks! ::: panels/background/cc-background-panel.c @@ +220,1 @@ + item = current_background == priv->current_background ? priv->current_background : priv->current_lock_background; this isn't really needed, we already have the right instance in current_background @@ +220,2 @@ + item = current_background == priv->current_background ? priv->current_background : priv->current_lock_background; + pixbuf = g_object_get_data (item, "pixbuf"); these calls need a G_OBJECT () cast macro around the first argument @@ +344,3 @@ data->monitor_rect.height); + /* invalidate existing cached pixbuf */ + g_object_steal_data (panel->priv->current_background, "pixbuf"); this would leak an existing cached pixbuf. setting the "pixbuf" data to NULL is what we want here
Rui, thanks for helping me with this PR!
*** Bug 782839 has been marked as a duplicate of this bug. ***