After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 695338 - runDialog: Remove the use of file monitors for file completion
runDialog: Remove the use of file monitors for file completion
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2013-03-07 02:32 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2013-03-07 21:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
runDialog: Remove tab-completion preloading (1.83 KB, patch)
2013-03-07 02:32 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
runDialog: Remove the use of file monitors for file completion (7.35 KB, patch)
2013-03-07 02:32 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2013-03-07 02:32:23 UTC
This was noted on IRC today -- simply opening the run dialog
and close it will make IO operations in the homedir slower,
because now there's a file notification pass in gnome-shell.

This is probably the placebo effect, but I find that jhbuilding
is actually a little bit faster with this patch.
Comment 1 Jasper St. Pierre (not reading bugmail) 2013-03-07 02:32:26 UTC
Created attachment 238269 [details] [review]
runDialog: Remove tab-completion preloading

This is iffy anyway, since we don't wait for the correct signal.
Just make the user press tab again, like they would do anyway.
Comment 2 Jasper St. Pierre (not reading bugmail) 2013-03-07 02:32:29 UTC
Created attachment 238270 [details] [review]
runDialog: Remove the use of file monitors for file completion

Launching the run dialog to open the looking glass or something
like that shouldn't install a bunch of file monitors that monitor
every IO change to the home and system directories.

Instead, simply scan all the paths when trying to complete.
Comment 3 Colin Walters 2013-03-07 02:37:32 UTC
Review of attachment 238270 [details] [review]:

Hmm...personally I think it'd be cool if Unix shells like bash actually made use of inotify rather than having to manually purge their cache with "hash -r" after changes.

Regardless wouldn't this patch be simpler if we just stopped monitoring for changes when the dialog closed?  Just need to shut down the monitors and call _init() again when the dialog opens.
Comment 4 Jasper St. Pierre (not reading bugmail) 2013-03-07 02:50:48 UTC
(In reply to comment #3)
> Review of attachment 238270 [details] [review]:
> 
> Hmm...personally I think it'd be cool if Unix shells like bash actually made
> use of inotify rather than having to manually purge their cache with "hash -r"
> after changes.

Slowing down I/O operations while we context switch into a full-stack GL client with a JavaScript interpreter isn't quite my idea of cool.

> Regardless wouldn't this patch be simpler if we just stopped monitoring for
> changes when the dialog closed?  Just need to shut down the monitors and call
> _init() again when the dialog opens.

Yes, but I don't think that's the right fix.
Comment 5 Jasper St. Pierre (not reading bugmail) 2013-03-07 02:51:41 UTC
That said, I think we need a better GFilenameCompleter API that's actually async instead of a-signal-sync, and can support a list of directories that's user-set.
Comment 6 Colin Walters 2013-03-07 13:47:30 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > Review of attachment 238270 [details] [review] [details]:
> > 
> > Hmm...personally I think it'd be cool if Unix shells like bash actually made
> > use of inotify rather than having to manually purge their cache with "hash -r"
> > after changes.
> 
> Slowing down I/O operations while we context switch into a full-stack GL client
> with a JavaScript interpreter isn't quite my idea of cool.

I don't understand this; what is slowing down I/O and how are context switches involved?  The fact that we're woken up while the run dialog is open?  

I agree it's broken that we're woken up when the run dialog *isn't* open.  But both approaches address that.
Comment 7 Alban Browaeys 2013-03-07 15:23:47 UTC
Thanks the slowdown is now quite rare : the leftover could well be realted to gnome-settings-daemon sending a broadcast to  gvc clients when a theme dir change (cf bug 691589 ).
Colin: the I/O are slowing due to various things (when one upgrade packages for system directories, when a file is written to in the home dir are the most obvious cases).
The issue is that when I/O load is high the monitors registered by runDialog (and kept even when runDialog is off) are constantly reading 1024 bytes on every file changed.
This I don't know why yet. 
The easiest way to reproduce is to strace the gnome shell with output sent to a file in the home dir ... in a few seconds 34000 reads of 1024 bytes on this file by the gio monitors registered by runDialog. The shell is frozen.
This is a testcase . In practice the shell freeze each time the load of the I/O is thigh (here when evolution start and read its cache - I have ML and system logs in various mailboxes that takes gigabytes and when upgrading system packages via apt).
Those are not at fault . Ie the I/O is expected. The issue is that at the same time gnome-shell try to do its own I/O in its monitors callbacks. And those are so numerous : menus directories, /etc for localtime file, home dir that they are always triggerring. So when anything else does I/O at the same time gnoem shell start to "freeze".

NB: I do not know why the strace output of the gnome shell shows read of 1024 of this output file at all. Monitoring directories seems not to require that read step.
Comment 8 Colin Walters 2013-03-07 18:58:47 UTC
Review of attachment 238269 [details] [review]:

Hmm.  I assume this code was solving a real problem, but it also looks kind of gross.  No objections to me since in the next patch we're probably fixing the bug this code was working around.
Comment 9 Colin Walters 2013-03-07 18:59:08 UTC
Review of attachment 238270 [details] [review]:

We discussed this in person, the inotify is kind of overkill.
Comment 10 Jasper St. Pierre (not reading bugmail) 2013-03-07 21:51:07 UTC
Attachment 238269 [details] pushed as d567576 - runDialog: Remove tab-completion preloading
Attachment 238270 [details] pushed as 40d9ed5 - runDialog: Remove the use of file monitors for file completion