GNOME Bugzilla – Bug 659689
gnome_rr_config_apply_from_filename_with_time refreshes the screen's state unnecessarily
Last modified: 2011-11-10 20:44:13 UTC
Created attachment 197140 [details] [review] Don't waste time doing a reprobe if the specified config file doesn't exist gnome_rr_config_apply_from_filename_with_time() makes an unnecessary XRRGetScreenResources roundtrip (via gnome_rr_screen_refresh) when the specified config file doesn't exist. This hurts session startup because gnome-settings-daemon calls this function 3 times on the critical path whilst iterating over the backup, intended and default configurations. By default, none of these configurations exist (if you've never changed any display settings), so the time is completely wasted. Currently, a XRRGetScreenResources roundtrip takes ~0.5s on my Intel hardware, which I believe is longer than it's meant to be due to a regression in the intel driver. However, even without that regression, XRRGetScreenResources isn't cheap. I've attached a patch which checks that the specified config file exists before doing anything else, which saves us a whole bunch of time at session startup in Ubuntu. However, I'm wondering if the gnome_rr_screen_refresh is even necessary? In the case of gnome-settings-daemon, it's only just created the GnomeRRScreen before doing this refresh at startup (so, there 2 XRRGetScreenResources round-trips in a row if you have a monitors.xml). There is no guarantee that the information on the server didn't change by the time this call finishes anyway (due to the async nature of X), so is it useful? At other times, gnome-desktop already updates it's state based on notifications from the server. Perhaps I'm misunderstanding something here though.
Almost certainly the wrong fix, even if calling XRRGetScreenResources() twice in quite succession should be avoided. Does this break the default boot monitor behaviour? http://git.gnome.org/browse/gnome-settings-daemon/tree/data/org.gnome.settings-daemon.plugins.xrandr.gschema.xml.in.in#n18
No, it doesn't break anything here, nor for anybody else running it
Review of attachment 197140 [details] [review]: This is not the best way to short-circuit this. This effectively creates stat() XRRGetScreenResources() open() and the stat() and open() are redundant checks. But the real question is, why is gnome_rr_config_apply_from_filename_with_time() refreshing the GnomeRRScreen in the first place? gnome_rr_screen_refresh() is only meant for the "Detect displays" command in the user interface; the library code shouldn't be doing refreshes on its own. That call to refresh() sounds like some old cruft from the very first implementation of gnome-rr-config.c. I'd remove that call instead, and test things for a bit - it should solve this particular performance problem. (Plus, Rodrigo just showed me a patch to make g-s-d use a single instance of GnomeRRScreen for all of its plugins, which sounds like a total win.)
It is true that gnome_rr_config_apply_from_filename_with_time() needs an up-to-date view of the screen's state, and that the state may not have been refreshed yet (via screen_on_event() -> screen_update()) if the event has not been processed and the app manages to call _apply_from_filename_w_t() before that. But even then, the app will get a "your timestamp is too old" error back, which is the right thing in any case. Refreshing the screen's state unconditionally (which could cause further events) seems totally wrong given X's async behavior. So, please remove the call to refresh(); that seems like the proper way to handle this.
Yes, I was hoping you would say that, but I just wasn't sure initially if removing the _refresh() was the right thing to do :)
Created attachment 201162 [details] [review] Don't call gnome_rr_screen_refresh when applying a config Hi, sorry, I went on vacation shortly after my last comment, then this sort-of dropped off my radar. Anyway, I've attached the patch we currently ship in Ubuntu which removes the call to _refresh
Thank you; this is perfect. I updated the docs to match the code, and pushed your patch to master as fee59a5e.