GNOME Bugzilla – Bug 592608
better handling of .desktop changes
Last modified: 2009-09-07 15:14:50 UTC
Right now the app system will reparse all .desktop files immediately on a change. It doesn't tell the callers exactly what changed either. I think the best short term hack would be to just set a dirty flag inside ShellAppSystem on a change, but emit the changed signal. Inside higher level things like the application well, also set a dirty flag. Then, only if the actor is visible, do a reload. Basically avoid all the .desktop parsing crazy if we're not in the overview. Intermediate term, we need to avoid parsing them all *twice*, once from Gio and once inside gnome-menus. Long term we really need a cache for .desktop files.
Per the conversation on 2009-08-20, this bug is generally about the shell locking up on upgrades. The same thing happened to me this weekend and I got a trace of gnome-shell stuck in an infinite loop reading all icons from /usr/share/icons/gnome/24x24/categories. The trace resembled read(file) = fd ioctl(fd) ioctl(fd) ioctl(fd) ioctl(fd) ioctl(fd) ioctl(fd) read(file) = fd [repeat]
*** Bug 593013 has been marked as a duplicate of this bug. ***
Created attachment 142345 [details] [review] Should fix the problem I was playing around with installers and couldn't stand this bug any longer. So far this patch has fixed the problem for me.
Comment on attachment 142345 [details] [review] Should fix the problem >From 2297bddfdad94982cc8cf53f50fb39f664621d0a Mon Sep 17 00:00:00 2001 >From: Jon Nettleton <jon.nettleton@gmail.com> >Date: Wed, 2 Sep 2009 11:45:58 -0700 >Subject: [PATCH] Added a lock and a timeout when getting the install-changed signal. > This was implemented in gnome-main-menu also. Could use a bit better description here...something like "We need to do application information reading in a timeout, because gnome-menus doesn't handle being called in a reentrant fashion well" or something. Also should copy&paste the comment in gnome-main-menus into the ShellAppSystem code or reference it. >--- > js/ui/appDisplay.js | 13 ++++++++++--- > 1 files changed, 10 insertions(+), 3 deletions(-) > >diff --git a/js/ui/appDisplay.js b/js/ui/appDisplay.js >index b313605..2e2bcbd 100644 >--- a/js/ui/appDisplay.js >+++ b/js/ui/appDisplay.js >@@ -185,10 +185,17 @@ AppDisplay.prototype = { > this._appMonitor = Shell.AppMonitor.get_default(); > this._appSystem = Shell.AppSystem.get_default(); > this._appsStale = true; >+ this._appsChangedQueued = false; > this._appSystem.connect('installed-changed', Lang.bind(this, function(appSys) { I think we should actually be doing this timeout inside ShellAppSystem, not in consumers.
Created attachment 142358 [details] [review] Updated patch This should work as well.
Comment on attachment 142358 [details] [review] Updated patch > >+ gboolean app_tree_changing; In other places I've more commonly added a "guint app_change_timeout_id" for example which has the return value from g_timeout_add. >- ShellAppSystem *self = SHELL_APP_SYSTEM (user_data); >+ ShellAppSystem *self = SHELL_APP_SYSTEM (user_data); >+ g_signal_emit (self, signals[INSTALLED_CHANGED], 0); >+ reread_menus (self); >+ self->priv->app_tree_changing = FALSE; Looks like this patch is using 4 spaces instead of 2? We're using GNU style in the shell C code, which is two spaces and braces on a new line. >+ if (!self->priv->app_tree_changing) { Braces on a new line here.
Comment on attachment 142358 [details] [review] Updated patch >+ priv->app_tree_changing = FALSE; >+ >+ gmenu_tree_add_monitor (priv->apps_tree, gmenu_tree_changed_timeout, self); >+ gmenu_tree_add_monitor (priv->settings_tree, gmenu_tree_changed_timeout, self); ... >- gmenu_tree_remove_monitor (priv->apps_tree, on_tree_changed, self); >- gmenu_tree_remove_monitor (priv->settings_tree, on_tree_changed, self); >+ gmenu_tree_remove_monitor (priv->apps_tree, gmenu_tree_changed, self); >+ gmenu_tree_remove_monitor (priv->settings_tree, gmenu_tree_changed, self); Shouldn't these two match? Why _timeout versus not? Which leads me to ask: >- g_signal_emit (self, signals[INSTALLED_CHANGED], 0); >+static void >+gmenu_tree_changed (GMenuTree *monitor, gpointer user_data) >+{ >+ on_tree_changed (user_data); >+} What's the purpose of this function?
ah sorry, I was reading that incorrectly. Thanks for sanity checking. I was reading that as removing the monitor ( incorrectly ) not adding a remove monitor. I will fix that quick and also the spacing.
Created attachment 142492 [details] [review] Read info in a timeout This should include all the necessary changes.
Comment on attachment 142492 [details] [review] Read info in a timeout Note that g_timeout_add returns an unsigned integer (guint), not gint. In practice it doesn't matter, but if you want to change it before committing it's someing to consider. Otherwise looks good, thanks a lot for taking a look at this issue! Hopefully in the future we can entirely kill off the gnome-menus dependency and avoid its bugs...
I didn't really consider it acceptable that over the years everybody who uses GMenu discovers the hard way that the monitors give vast amounts of duplicate notification. So, I went ahead and created a patch for bug 172046. None-the-less, something along these lines would be a good idea. First, because it's too much of a problem to leave around until that patch makes its way into all distros, and second because we need to compress across apps_tree and settings_tree, and third because we probably do want a timeout compress to handle a bunch of changes being made at once. The comment: + /* + This method only gets called on the first change (gmenu appears to ignore subsequent) until + we reget the root dir which we can't do in this method because if we do for some reason this + method then gets called multiple times for one actual change. This actually is okay because + it's probably a good idea to wait a couple seconds to regenerate the categories in case there + are multiple quick changes being made, no sense regenerating multiple times. + */ Needs some rewriting (and formatting fix - every line should start with a *.) This method only gets called on the first change (gmenu appears to ignore subsequent) until we reget the root dir which we can't do in this method Is a bit of a red herring - while true, it's not very useful since it's saying that notifications are compressed if you don't do anything in your notification callback. Suggested rewrite: GMenu currently gives us a separate notification on the entire menu tree for each node in the tree that might potentially have changed. (See http://bugzilla.gnome.org/show_bug.cgi?id=172046.) We need to compress these to avoid doing large extra amounts of work. Even when that bug is fixed, compression is still useful; for one thing we want to need to compress across notifications of changes to the settings tree. Second we want to compress if multiple changes are made to the desktop files at different times but in short succession. Beyond that, there are two problems that you can see looking at the traces. These should be fixed separately. ui/appDisplay.js:MenuItem needs to use the texture cache. (This isn't actually a *huge* problem since it only affects 15 or so textures, but it's ugly to recreate them) 553503dace358ffcc83c02a3ecd479d32f8f4743 Remove the timeout for checking if an item is drawn under the pointer Is a bad idea - you should not call get_actor_at_pos out of a function creating a tree, since it will cause the entire stage to be layed-out synchronously at that point. So, we start recreating the menus, do a full layout, create some more, do another layout. The appropriate thing to do is probably to use an idle function with the priority: META_PRIORITY_BEFORE_REDRAW (G_PRIORITY_HIGH_IDLE + 40 (mutter/src/include/common.h has the big list of priorities)
Created attachment 142508 [details] [review] Make checking if an item is under the pointer asynchonous again Checking if an item is under the pointer by calling stage.get_actor_at_pos() synchronously will trigger a too-early allocation of the stage. Use an idle at Meta.PRIORITY_BEFORE_REDRAW. (Before 553503d it was using a 5 msec timeout, 553503d made it synchronous.)
Comment on attachment 142508 [details] [review] Make checking if an item is under the pointer asynchonous again Looks right, but I wonder if there is a better way to handle this kind of thing; is the problem that we won't get enter-event if the pointer doesn't move an an actor appears under it? I bet this bug is also the source of the occasional one where the recent files overflows its area.
(In reply to comment #13) > (From update of attachment 142508 [details] [review]) > Looks right, but I wonder if there is a better way to handle this kind of > thing; is the problem that we won't get enter-event if the pointer doesn't move > an an actor appears under it? Yes. This should be fixed in clutter, but it would cause a pick to be done for every animation frame which probably could slow clutter down by 10-20% overall. So it's possible that it won't get fixed until clutter has non-paint-the-scene based picking. > I bet this bug is also the source of the occasional one where the recent files > overflows its area. if clutter is doing it's job, the early allocation should just cause extra work, but it's possible that there are some corner cases triggered.
Comment on attachment 142508 [details] [review] Make checking if an item is under the pointer asynchonous again Attachment 142508 [details] pushed as b7b4c54 - Make checking if an item is under the pointer asynchonous again
Created attachment 142511 [details] [review] Compress notifications of menu changes with a timeout. This is Jon's patch with: - Colin's guint change - My comment change I'm going to go ahead and push it this way so we get it into today's release, since it's a pretty bad bug.
Attachment 142511 [details] pushed as 0a29cf6 - Compress notifications of menu changes with a timeout. I filed the texture cache item as bug 594183, so I think we can close this one now.
Hey guys we need this patch added to the code. Without returning false we are doing a much worse endless loop of menu tree checking. Sorry about missing this. diff --git a/src/shell-app-system.c b/src/shell-app-system.c index 6a9d74e..67334af 100644 --- a/src/shell-app-system.c +++ b/src/shell-app-system.c @@ -57,7 +57,7 @@ struct _ShellAppSystemPrivate { }; static void shell_app_system_finalize (GObject *object); -static void on_tree_changed (gpointer user_data); +static gboolean on_tree_changed (gpointer user_data); static void on_tree_changed_cb (GMenuTree *tree, gpointer user_data); static void reread_menus (ShellAppSystem *self); static void on_favorite_apps_changed (GConfClient *client, guint id, GConfEntry *entry, gpointer @@ -415,13 +415,14 @@ reread_menus (ShellAppSystem *self) cache_by_id (self, self->priv->cached_settings, TRUE); } -static void +static gboolean on_tree_changed (gpointer user_data) { ShellAppSystem *self = SHELL_APP_SYSTEM (user_data); g_signal_emit (self, signals[INSTALLED_CHANGED], 0); reread_menus (self); self->priv->app_change_timeout_id = 0; + return FALSE; } static void
Created attachment 142627 [details] [review] Fix on_tree_changed being called in a timed endless loop. Convert on_tree_changed to a gboolean returning false. This fixes commit 0a29cf61955935ca1e7d3a48ccd38164ec9c80af and g_timer_add_full running in an endless loop.
Comment on attachment 142627 [details] [review] Fix on_tree_changed being called in a timed endless loop. Jon asked me to commit this since no one else was around Today, and it's pretty obviously correct.