GNOME Bugzilla – Bug 765486
builtinicon: avoid calculating font-metrics in vast majority of cases
Last modified: 2016-04-25 20:14:00 UTC
We do an awful lot of font metrics calculation that can simply go away by caching the result.
Created attachment 326615 [details] [review] builtinicon: avoid calculating font-metrics in vast majority of cases We perform lots of gadget allocations that require allocating a GtkBuiltinIcon. One notable example is the scrollbar for a scrolled window. In the process of doing this, we often calculate baseline information that isn't necessary. With how much this code path gets exercised, its worth catching the result for the common case, which is that the font-description has not changed and we are using the default language the application was started with. This simply caches the previous result and verifies that we can reuse it with pango_font_description_hash() and a simple language check. Numbers below are scrolling through a textview with GDK_KEY_Down. Before: SELF CUMULATIVE FUNCTION [ 0.08%] [ 9.26%] gtk_builtin_icon_get_preferred_size [ 0.01%] [ 8.82%] pango_context_get_metrics [ 0.02%] [ 0.16%] gtk_widget_get_pango_context [ 0.06%] [ 0.06%] pango_context_get_language [ 0.01%] [ 0.02%] g_type_check_instance_cast [ 0.02%] [ 0.02%] strlen [ 0.02%] [ 0.02%] pango_context_get_font_description [ 0.02%] [ 0.02%] g_list_foreach [ 0.01%] [ 0.01%] gtk_css_style_get_value [ 0.01%] [ 0.01%] itemize_with_font [ 0.01%] [ 0.01%] pango_context_get_type [ 0.01%] [ 0.01%] get_base_metrics [ 0.00%] [ 0.01%] pango_font_metrics_unref [ 0.01%] [ 0.01%] g_list_free [ 0.01%] [ 0.01%] gtk_builtin_icon_get_type After: SELF CUMULATIVE FUNCTION [ 0.08%] [ 0.18%] gtk_builtin_icon_get_preferred_size [ 0.02%] [ 0.02%] pango_font_description_hash [ 0.00%] [ 0.02%] gtk_widget_get_pango_context [ 0.00%] [ 0.02%] g_object_get_qdata [ 0.00%] [ 0.02%] g_datalist_id_get_data [ 0.02%] [ 0.02%] gtk_builtin_icon_get_type [ 0.01%] [ 0.01%] pango_context_get_font_description [ 0.00%] [ 0.01%] - - kernel - - [ 0.01%] [ 0.01%] pango_context_get_language [ 0.00%] [ 0.01%] gtk_css_style_get_value [ 0.00%] [ 0.01%] gtk_css_gadget_get_style
Created attachment 326616 [details] [review] avoid calculating font-metrics in vast majority of cases Move our metrics unref inside the slow path.
Looks like a nice improvement, but I think the cache invalidation should be tied to css invalidation. Add a style_changed vfunc, and check for GTK_CSS_AFFECTS_FONT. I don't think you need to check for language changes; I don't think the language can ever by something other than the default here.
Review of attachment 326616 [details] [review]: .
Created attachment 326661 [details] [review] builtinicon: avoid calculating font-metrics in vast majority of cases We perform lots of gadget allocations that require allocating a GtkBuiltinIcon. One notable example is the scrollbar for a scrolled window. In the process of doing this, we often calculate baseline information that isn't necessary. With how much this code path gets exercised, its worth catching the result for the common case, which is that the font-description has not changed and we are using the default language the application was started with. This simply caches the previous result and verifies that we can reuse it with pango_font_description_hash() and a simple language check. Numbers below are scrolling through a textview with GDK_KEY_Down. Before: SELF CUMULATIVE FUNCTION [ 0.08%] [ 9.26%] gtk_builtin_icon_get_preferred_size [ 0.01%] [ 8.82%] pango_context_get_metrics [ 0.02%] [ 0.16%] gtk_widget_get_pango_context [ 0.06%] [ 0.06%] pango_context_get_language [ 0.01%] [ 0.02%] g_type_check_instance_cast [ 0.02%] [ 0.02%] strlen [ 0.02%] [ 0.02%] pango_context_get_font_description [ 0.02%] [ 0.02%] g_list_foreach [ 0.01%] [ 0.01%] gtk_css_style_get_value [ 0.01%] [ 0.01%] itemize_with_font [ 0.01%] [ 0.01%] pango_context_get_type [ 0.01%] [ 0.01%] get_base_metrics [ 0.00%] [ 0.01%] pango_font_metrics_unref [ 0.01%] [ 0.01%] g_list_free [ 0.01%] [ 0.01%] gtk_builtin_icon_get_type After: SELF CUMULATIVE FUNCTION [ 0.08%] [ 0.18%] gtk_builtin_icon_get_preferred_size [ 0.02%] [ 0.02%] pango_font_description_hash [ 0.00%] [ 0.02%] gtk_widget_get_pango_context [ 0.00%] [ 0.02%] g_object_get_qdata [ 0.00%] [ 0.02%] g_datalist_id_get_data [ 0.02%] [ 0.02%] gtk_builtin_icon_get_type [ 0.01%] [ 0.01%] pango_context_get_font_description [ 0.00%] [ 0.01%] - - kernel - - [ 0.01%] [ 0.01%] pango_context_get_language [ 0.00%] [ 0.01%] gtk_css_style_get_value [ 0.00%] [ 0.01%] gtk_css_gadget_get_style
It does indeed look better with the vfunc. Thanks for the suggestion!
Review of attachment 326661 [details] [review]: ::: gtk/gtkbuiltinicon.c @@ +110,1 @@ + if (priv->last_font_desc_hash != font_desc_hash) I don't really think you need to hash the font at all - you could just have a strikethough_valid boolean and set that to FALSE in the style_changed vfunc. @@ +126,2 @@ if (natural_baseline) *natural_baseline = *minimum_baseline; Another possible improvement here would be to only to the hash check if minimum_baseline is not NULL.
Attachment 326661 [details] pushed as f165bbd - builtinicon: avoid calculating font-metrics in vast majority of cases