From 87afb02e50c7a636a508cf619beb68a46168e54b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 6 May 2019 18:28:40 -0500 Subject: [PATCH 1/5] st-theme: Use CRStyleSheet app_data instead of hash map Use the CRStyleSheet field to save stylesheet details instead of using an extra hash table. In this way we can access to the stylesheet file faster without having to lookup it. Define a destroy function so that we can automatically remove the data when the container hash table is destroyed. Fixes https://gitlab.gnome.org/GNOME/gnome-shell/issues/1265 https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/536 --- src/st/st-theme.c | 91 +++++++++++++++++++++++++++++++---------------- 1 file changed, 60 insertions(+), 31 deletions(-) diff --git a/src/st/st-theme.c b/src/st/st-theme.c index b567f7e5e3..208f536c5b 100644 --- a/src/st/st-theme.c +++ b/src/st/st-theme.c @@ -66,11 +66,16 @@ struct _StTheme GSList *custom_stylesheets; GHashTable *stylesheets_by_file; - GHashTable *files_by_stylesheet; CRCascade *cascade; }; +typedef struct _StyleSheetData +{ + GFile *file; + gboolean extension_stylesheet; +} StyleSheetData; + enum { PROP_0, @@ -106,12 +111,25 @@ file_equal0 (GFile *file1, return g_file_equal (file1, file2); } +static void +stylesheet_destroy (CRStyleSheet *stylesheet) +{ + if (stylesheet->app_data) + { + g_slice_free (StyleSheetData, stylesheet->app_data); + stylesheet->app_data = NULL; + } + + cr_stylesheet_unref (stylesheet); +} + static void st_theme_init (StTheme *theme) { - theme->stylesheets_by_file = g_hash_table_new_full (g_file_hash, (GEqualFunc) g_file_equal, - (GDestroyNotify)g_object_unref, (GDestroyNotify)cr_stylesheet_unref); - theme->files_by_stylesheet = g_hash_table_new (g_direct_hash, g_direct_equal); + theme->stylesheets_by_file = + g_hash_table_new_full (g_file_hash, (GEqualFunc) g_file_equal, + (GDestroyNotify) g_object_unref, + (GDestroyNotify) stylesheet_destroy); } static void @@ -205,9 +223,6 @@ parse_stylesheet (GFile *file, return NULL; } - /* Extension stylesheet */ - stylesheet->app_data = GUINT_TO_POINTER (FALSE); - return stylesheet; } @@ -234,19 +249,30 @@ parse_stylesheet_nofail (GFile *file) return result; } -static void +static gboolean insert_stylesheet (StTheme *theme, GFile *file, CRStyleSheet *stylesheet) { + StyleSheetData *stylesheet_data; + if (stylesheet == NULL) - return; + return FALSE; - g_object_ref (file); - cr_stylesheet_ref (stylesheet); + if (g_hash_table_contains (theme->stylesheets_by_file, file)) + { + cr_stylesheet_unref (stylesheet); + return FALSE; + } - g_hash_table_insert (theme->stylesheets_by_file, file, stylesheet); - g_hash_table_insert (theme->files_by_stylesheet, stylesheet, file); + stylesheet_data = g_slice_new0 (StyleSheetData); + stylesheet_data->file = file; + stylesheet->app_data = stylesheet_data; + + cr_stylesheet_ref (stylesheet); + g_hash_table_insert (theme->stylesheets_by_file, + g_object_ref (file), stylesheet); + return TRUE; } gboolean @@ -255,14 +281,15 @@ st_theme_load_stylesheet (StTheme *theme, GError **error) { CRStyleSheet *stylesheet; + StyleSheetData *stylesheet_data; stylesheet = parse_stylesheet (file, error); - if (!stylesheet) + if (!insert_stylesheet (theme, file, stylesheet)) return FALSE; - stylesheet->app_data = GUINT_TO_POINTER (TRUE); + stylesheet_data = stylesheet->app_data; + stylesheet_data->extension_stylesheet = TRUE; - insert_stylesheet (theme, file, stylesheet); cr_stylesheet_ref (stylesheet); theme->custom_stylesheets = g_slist_prepend (theme->custom_stylesheets, stylesheet); g_signal_emit (theme, signals[STYLESHEETS_CHANGED], 0); @@ -283,9 +310,8 @@ st_theme_unload_stylesheet (StTheme *theme, if (!g_slist_find (theme->custom_stylesheets, stylesheet)) return; - theme->custom_stylesheets = g_slist_remove (theme->custom_stylesheets, stylesheet); g_hash_table_remove (theme->stylesheets_by_file, file); - g_hash_table_remove (theme->files_by_stylesheet, stylesheet); + theme->custom_stylesheets = g_slist_remove (theme->custom_stylesheets, stylesheet); cr_stylesheet_unref (stylesheet); g_signal_emit (theme, signals[STYLESHEETS_CHANGED], 0); } @@ -306,9 +332,10 @@ st_theme_get_custom_stylesheets (StTheme *theme) for (iter = theme->custom_stylesheets; iter; iter = iter->next) { CRStyleSheet *stylesheet = iter->data; - GFile *file = g_hash_table_lookup (theme->files_by_stylesheet, stylesheet); + StyleSheetData *stylesheet_data = stylesheet->app_data; - result = g_slist_prepend (result, g_object_ref (file)); + if (stylesheet_data && stylesheet_data->file) + result = g_slist_prepend (result, g_object_ref (stylesheet_data->file)); } return result; @@ -350,7 +377,6 @@ st_theme_finalize (GObject * object) theme->custom_stylesheets = NULL; g_hash_table_destroy (theme->stylesheets_by_file); - g_hash_table_destroy (theme->files_by_stylesheet); g_clear_object (&theme->application_stylesheet); g_clear_object (&theme->theme_stylesheet); @@ -877,18 +903,20 @@ add_matched_properties (StTheme *a_this, if (import_rule->url->stryng && import_rule->url->stryng->str) { + CRStyleSheet *sheet; file = _st_theme_resolve_url (a_this, a_nodesheet, import_rule->url->stryng->str); - import_rule->sheet = parse_stylesheet (file, NULL); - } + sheet = parse_stylesheet (file, NULL); - if (import_rule->sheet) - { - insert_stylesheet (a_this, file, import_rule->sheet); - /* refcount of stylesheets starts off at zero, so we don't need to unref! */ + if (insert_stylesheet (a_this, file, sheet)) + import_rule->sheet = sheet; + + /* refcount of stylesheets starts off at zero, so we don't + * need to unref! */ } - else + + if (!import_rule->sheet) { /* Set a marker to avoid repeatedly trying to parse a non-existent or * broken stylesheet @@ -962,12 +990,12 @@ static inline int get_origin (const CRDeclaration * decl) { enum CRStyleOrigin origin = decl->parent_statement->parent_sheet->origin; - gboolean is_extension_sheet = GPOINTER_TO_UINT (decl->parent_statement->parent_sheet->app_data); + StyleSheetData *sheet_data = decl->parent_statement->parent_sheet->app_data; if (decl->important) origin += ORIGIN_OFFSET_IMPORTANT; - if (is_extension_sheet) + if (sheet_data && sheet_data->extension_stylesheet) origin += ORIGIN_OFFSET_EXTENSION; return origin; @@ -1046,8 +1074,9 @@ _st_theme_resolve_url (StTheme *theme, else if (base_stylesheet != NULL) { GFile *base_file = NULL, *parent; + StyleSheetData *stylesheet_data = base_stylesheet->app_data; - base_file = g_hash_table_lookup (theme->files_by_stylesheet, base_stylesheet); + base_file = stylesheet_data->file; /* This is an internal function, if we get here with a bad @base_stylesheet we have a problem. */ -- 2.24.1 From 0974a3290aa2951ada3c9f2adceb2c65c2321849 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 6 May 2019 18:33:36 -0500 Subject: [PATCH 2/5] st-theme: Use newer functions to finalize objects Use g_slist_free_full on custom stylesheets list and clear the hashtable and its pointer using g_clear_pointer and g_hash_table_destroy. https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/536 --- src/st/st-theme.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/st/st-theme.c b/src/st/st-theme.c index 208f536c5b..2d51420a20 100644 --- a/src/st/st-theme.c +++ b/src/st/st-theme.c @@ -372,11 +372,11 @@ st_theme_finalize (GObject * object) { StTheme *theme = ST_THEME (object); - g_slist_foreach (theme->custom_stylesheets, (GFunc) cr_stylesheet_unref, NULL); - g_slist_free (theme->custom_stylesheets); - theme->custom_stylesheets = NULL; + g_clear_pointer (&theme->stylesheets_by_file, g_hash_table_destroy); - g_hash_table_destroy (theme->stylesheets_by_file); + g_slist_free_full (theme->custom_stylesheets, + (GDestroyNotify) cr_stylesheet_unref); + theme->custom_stylesheets = NULL; g_clear_object (&theme->application_stylesheet); g_clear_object (&theme->theme_stylesheet); -- 2.24.1 From 801fd1884af2011ec6981e4d7844aac6f8dbecf0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Tue, 7 May 2019 01:59:11 -0500 Subject: [PATCH 3/5] st-theme: Remove custom stylesheets list Since we already mark the stylesheet laded by extensions in the data, we don't need to use another list to go trough these as we can just iterate over the hash table with a minimum overhead, as this will normally contain just one default stylesheet and all the extension stylesheets anyways. https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/536 --- src/st/st-theme.c | 37 +++++++++++++++++++------------------ 1 file changed, 19 insertions(+), 18 deletions(-) diff --git a/src/st/st-theme.c b/src/st/st-theme.c index 2d51420a20..9f6b2f6e35 100644 --- a/src/st/st-theme.c +++ b/src/st/st-theme.c @@ -63,7 +63,6 @@ struct _StTheme GFile *application_stylesheet; GFile *default_stylesheet; GFile *theme_stylesheet; - GSList *custom_stylesheets; GHashTable *stylesheets_by_file; @@ -290,8 +289,6 @@ st_theme_load_stylesheet (StTheme *theme, stylesheet_data = stylesheet->app_data; stylesheet_data->extension_stylesheet = TRUE; - cr_stylesheet_ref (stylesheet); - theme->custom_stylesheets = g_slist_prepend (theme->custom_stylesheets, stylesheet); g_signal_emit (theme, signals[STYLESHEETS_CHANGED], 0); return TRUE; @@ -302,17 +299,17 @@ st_theme_unload_stylesheet (StTheme *theme, GFile *file) { CRStyleSheet *stylesheet; + StyleSheetData *stylesheet_data; stylesheet = g_hash_table_lookup (theme->stylesheets_by_file, file); - if (!stylesheet) + if (!stylesheet || !stylesheet->app_data) return; - if (!g_slist_find (theme->custom_stylesheets, stylesheet)) + stylesheet_data = stylesheet->app_data; + if (!stylesheet_data->extension_stylesheet) return; g_hash_table_remove (theme->stylesheets_by_file, file); - theme->custom_stylesheets = g_slist_remove (theme->custom_stylesheets, stylesheet); - cr_stylesheet_unref (stylesheet); g_signal_emit (theme, signals[STYLESHEETS_CHANGED], 0); } @@ -327,14 +324,17 @@ GSList* st_theme_get_custom_stylesheets (StTheme *theme) { GSList *result = NULL; - GSList *iter; + GHashTableIter iter; + gpointer value; - for (iter = theme->custom_stylesheets; iter; iter = iter->next) + g_hash_table_iter_init (&iter, theme->stylesheets_by_file); + + while (g_hash_table_iter_next (&iter, NULL, &value)) { - CRStyleSheet *stylesheet = iter->data; + CRStyleSheet *stylesheet = value; StyleSheetData *stylesheet_data = stylesheet->app_data; - if (stylesheet_data && stylesheet_data->file) + if (stylesheet_data && stylesheet_data->extension_stylesheet) result = g_slist_prepend (result, g_object_ref (stylesheet_data->file)); } @@ -374,10 +374,6 @@ st_theme_finalize (GObject * object) g_clear_pointer (&theme->stylesheets_by_file, g_hash_table_destroy); - g_slist_free_full (theme->custom_stylesheets, - (GDestroyNotify) cr_stylesheet_unref); - theme->custom_stylesheets = NULL; - g_clear_object (&theme->application_stylesheet); g_clear_object (&theme->theme_stylesheet); g_clear_object (&theme->default_stylesheet); @@ -1030,7 +1026,7 @@ _st_theme_get_matched_properties (StTheme *theme, enum CRStyleOrigin origin = 0; CRStyleSheet *sheet = NULL; GPtrArray *props = g_ptr_array_new (); - GSList *iter; + GHashTableIter iter; g_return_val_if_fail (ST_IS_THEME (theme), NULL); g_return_val_if_fail (ST_IS_THEME_NODE (node), NULL); @@ -1044,8 +1040,13 @@ _st_theme_get_matched_properties (StTheme *theme, add_matched_properties (theme, sheet, node, props); } - for (iter = theme->custom_stylesheets; iter; iter = iter->next) - add_matched_properties (theme, iter->data, node, props); + g_hash_table_iter_init (&iter, theme->stylesheets_by_file); + while (g_hash_table_iter_next (&iter, NULL, (gpointer) &sheet)) + { + if (sheet->app_data && + ((StyleSheetData *) sheet->app_data)->extension_stylesheet) + add_matched_properties (theme, sheet, node, props); + } /* We count on a stable sort here so that later declarations come * after earlier declarations */ -- 2.24.1 From 5b66ecb1019f3ba32aa05562a8b58ebd95702669 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 6 May 2019 18:56:49 -0500 Subject: [PATCH 4/5] st-theme: Ref stylesheet using facility function Define stylesheet_ref as a wrapper of cr_stylesheet_ref that returns the ref'ed instance so that we can use this on the caller function to make clear what we're passing. https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/536 --- src/st/st-theme.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/st/st-theme.c b/src/st/st-theme.c index 9f6b2f6e35..30733b6fd5 100644 --- a/src/st/st-theme.c +++ b/src/st/st-theme.c @@ -110,6 +110,13 @@ file_equal0 (GFile *file1, return g_file_equal (file1, file2); } +static inline CRStyleSheet * +stylesheet_ref (CRStyleSheet *stylesheet) +{ + cr_stylesheet_ref (stylesheet); + return stylesheet; +} + static void stylesheet_destroy (CRStyleSheet *stylesheet) { @@ -268,9 +275,8 @@ insert_stylesheet (StTheme *theme, stylesheet_data->file = file; stylesheet->app_data = stylesheet_data; - cr_stylesheet_ref (stylesheet); g_hash_table_insert (theme->stylesheets_by_file, - g_object_ref (file), stylesheet); + g_object_ref (file), stylesheet_ref (stylesheet)); return TRUE; } -- 2.24.1 From bf5ea4dc4fbcf09c77a830ae9a227660e1c72d7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Trevisan=20=28Trevi=C3=B1o=29?= Date: Mon, 6 May 2019 18:40:13 -0500 Subject: [PATCH 5/5] st-theme: Use glib auto free/ptr features Use g_autofree and g_autoptr for managing memory in a smarter way. https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/536 --- src/st/st-theme.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/src/st/st-theme.c b/src/st/st-theme.c index 30733b6fd5..4c8730a4ce 100644 --- a/src/st/st-theme.c +++ b/src/st/st-theme.c @@ -205,7 +205,7 @@ parse_stylesheet (GFile *file, { enum CRStatus status; CRStyleSheet *stylesheet; - char *contents; + g_autofree char *contents = NULL; gsize length; if (file == NULL) @@ -218,14 +218,11 @@ parse_stylesheet (GFile *file, length, CR_UTF_8, &stylesheet); - g_free (contents); - if (status != CR_OK) { - char *uri = g_file_get_uri (file); + g_autofree char *uri = g_file_get_uri (file); g_set_error (error, G_IO_ERROR, G_IO_ERROR_FAILED, "Error parsing stylesheet '%s'; errcode:%d", uri, status); - g_free (uri); return NULL; } @@ -901,7 +898,7 @@ add_matched_properties (StTheme *a_this, if (import_rule->sheet == NULL) { - GFile *file = NULL; + g_autoptr (GFile) file = NULL; if (import_rule->url->stryng && import_rule->url->stryng->str) { @@ -925,9 +922,6 @@ add_matched_properties (StTheme *a_this, */ import_rule->sheet = (CRStyleSheet *) - 1; } - - if (file) - g_object_unref (file); } if (import_rule->sheet != (CRStyleSheet *) - 1) @@ -1070,20 +1064,19 @@ _st_theme_resolve_url (StTheme *theme, CRStyleSheet *base_stylesheet, const char *url) { - char *scheme; + g_autofree char *scheme = NULL; GFile *resource; if ((scheme = g_uri_parse_scheme (url))) { - g_free (scheme); resource = g_file_new_for_uri (url); } else if (base_stylesheet != NULL) { - GFile *base_file = NULL, *parent; StyleSheetData *stylesheet_data = base_stylesheet->app_data; + GFile *base_file = stylesheet_data->file; + g_autoptr (GFile) parent = NULL; - base_file = stylesheet_data->file; /* This is an internal function, if we get here with a bad @base_stylesheet we have a problem. */ @@ -1091,8 +1084,6 @@ _st_theme_resolve_url (StTheme *theme, parent = g_file_get_parent (base_file); resource = g_file_resolve_relative_path (parent, url); - - g_object_unref (parent); } else { -- 2.24.1