GNOME Bugzilla – Bug 354213
Performance: Cut down on context switches (_NET_WM_USER_TIME_WINDOW)
Last modified: 2007-04-01 03:44:59 UTC
Matthias found out that metacity and all processes using libwnck are awoken for every keystroke or mouse button press on an application. This adds lots of context switches. This is because of apps updating _NET_WM_USER_TIME upon every keystroke. Matthias said in email: So, if the extra indirection sounds ok to you, I'll work out a proposal for the ewmh and post it on wm-spec-list. I hope you can tackle the metacity side, since I don't know the code well enough to quickly findwhere it reads user time. To summarize, the required metacity changes are: 1) advertise support for _NET_WM_USER_TIME_WINDOW 2) if this property is set on a toplevel, read _NET_WM_USER_TIME from that window, rather than the toplevel itself Btw, I wondered if it would be worthwhile to not select for property notify on the window, and instead onlyread the user time if it is actually needed. Assuming metacity only uses user-time for focus policy decisions, it should be needed much less often than it gets updated by clients...
My proposal is here: http://mail.gnome.org/archives/wm-spec-list/2006-September/msg00000.html
I have committed the _NET_WM_USER_TIME_WINDOW change to the EWMH now.
Elijah, any news on this ? I recently discovered that gnome-settings-daemon also listens for PropertyChange on all toplevels (libxklavier madness). I'd like to get this implemented for 2.18 - the GTK+ side is all done.
Hi Matthias, I've been buried under trying to finish my dissertation and have been almost completely absent for a while. However, I finally defended my dissertation last Wednesday (WOOHOO!) so I'll be able to get some stuff done again. I don't have time right at the moment to work on it (long back log and various odds and ends competing for time currently), but I am catching up and I believe I should be able to get to it before the end of the month. It might be nice if you or someone could ping me in a couple weeks, just in case. :)
Cool, congratulations on the dissertation.
I'm not sure I know enough about EWMH stuff to help here, but I'm more than willing to help if I can.
Created attachment 85294 [details] [review] Completely untested patch Matthias: Where's the gtk+ patch you created? It didn't seem to be applied to trunk and I couldn't find it anywhere to test with. Anyway, I whipped up this patch, which I think probably has most the needed stuff, but I'd be surprised if it works without some tweaks/fixes. I was too lazy to try to duplicate the gtk+ patch; send it my way and I'll test this out and fix it up.
Hmm, I was sure I had this committed to trunk. I need to dig out the patch tomorrow.
Created attachment 85368 [details] [review] new patch I could find absolutely no trace of my old patch, therefore I have quickly recreated an untested gtk patch.
Created attachment 85427 [details] [review] Fix one crasher bug I didn't get much time to test, but I did find one bug in this patch and can verify that the patch doesn't quite work. Some kind of nasty hang. Will hopefully have some time to check tomorrow or Thursday.
Created attachment 85428 [details] [review] Correct a couple bugs in the gtk+ patch In very brief testing, I think this patch works. Fixes two minor problems (including a crasher) in the previous gtk+ patch.
Created attachment 85429 [details] [review] Upload the right gtk+ patch Previous upload was a version with only one of the two fixes; oops.
Created attachment 85496 [details] [review] Make sure to deregister extra window, only respond to property notify events instead of also unmap events Passes basic testing with a simplified gtk+ patch I'll attach in a minute.
Created attachment 85497 [details] [review] Simplified gtk+ patch, ditching backwards compatibility Metacity seemed to hang (at launch time) with the other gtk+ patch; it didn't seem to like the gdk_x11_screen_supports... calls. So I ripped them out for testing purposes. Found that toplevel->focus_window is sometimes None (input only windows) and thus isn't a valid window. That makes our setting of _NET_WM_USER_TIME_WINDOW for such windows buggy, and our setting of _NET_WM_USER_TIME on the None window kinda crashy. Used a simple check here to avoid the crash, but it needs a better solution. Anyway, this gtk+ and corresponding metacity patch seem to be at least minimally functional. Don't have time to test further at the moment.
Created attachment 85550 [details] [review] Fixed up metacity patch, with some debug spew Okay, after a little bit more testing I think this metacity patch is finally functional. Extra testers would be nice (the debug spew can help you test). All I think we have left is fixing up the gtk+ patch. Fixing up the gtk+ patch means (1) determining whether to use something other than toplevel->focus_window or changing the gtk+ logic to always create that window, and (2) adding backwards compatibility back in, i.e. finding out why metacity freezes when gtk+ has the gdk_x11_screen_supports_net_wm_hint() calls (which is actually an amusing problem -- metacity uses gtk+ for the theme code, so by having the theme code have gtk+ do stuff, we run into a problem where gtk+ needs to figure out what the window manager supports and tries to figure out in a way that ignores that in this particular case it _is_ the window manager!).
Created attachment 85570 [details] [review] new patch Here is how I think the gtk patch _should_ look like. Since I think toplevel input-only windows are pretty rare, it is probably fine to just set the user-time on the toplevel there. Also note that I always set the user-time-window, regardless of the wm support, and only check the wm support in set_user_time. That should allow wm changes to work nicely, and a wm that doesn't support user time windows will hopefully just ignore the property. Sadly, I can confirm that this patch makes metacity hang when trying to change workspaces with c-a-left/right. Is the window that it pops up there a toplevel input-only window by any chance ?
Here is a stacktrace from when metacity gets stuck:
+ Trace 123566
Other than this ugly problem, I can confirm that the patches work together to stop the wncklet from waking up for every keypress.
Don't know if it relevant, but I notice that the metacity patch tries to load both the user time and the user time window property from the toplevel window initially (see the setting of initial_props. Getting the initial user time in this way will fail if the user time window is used by the client.
I'm completely stumped by this hanging. Interestingly, it's not 100% either; I have been able to switch workspaces sometimes without the hang, though most often the hang occurs. So there's got to be some kind of race being triggered. I don't think the workspace switcher popup is a toplevel input-only window. It's created in metacity/src/tabpopup.c:meta_ui_tab_popup_new() if you want to double check and verify for me. I'm not following comment 19.
Yeah, I noticed later that you also get the user time when registering a new user time window. Ignore comment 19.
as far as I can guess the hang could only mean two things: - somehow no property event is generated by the XChangeProperty (don't see how) - some other client has a server grab, and is blocking on metacity, so it deadlocks (seems impossible, since metacity itself does a display grab in idle_calc_showing) Strange. btw, doing a get_current_time_roundtrip for every window when switching workspaces seems pretty harsh performance-wise. Might be nice to figure out how to do that only once.
Two workarounds that remove this hang: 1) Adding a "return CurrentTime" to the top of meta_display_get_current_time_roundtrip() 2) Removing the call to gdk_x11_screen_supports_net_wm_hint() in gdk_x11_window_set_user_time. These two workarounds haven't helped me track down the cause at all, but maybe it'll help someone else?
theory: try adding PropertyChangeMask to the XSelectInput() in gdkevents-x11.c:fetch_net_wm_check_window(). my theory is that since the wm check window is the same as metacity's display->leader_window, gtk is removing PropertyChangeMask from the leader window, which means we don't get the PropertyNotify with the timestamp (and block forever waiting for it).
Created attachment 85596 [details] [review] Woot! It works! You're a genius, Havoc.
Created attachment 85597 [details] [review] Metacity patch without the debug spew (and without the accidental inclusion of a gnome-love bugfix I said I wouldn't be providing until someone else fixed it--oops)
Ah, thats plausible. Feel free to commit the GTK+ patch to trunk, Elijah. Should we open another bug for Havoc's observation about excessive roundtrips when switching workspaces ?
Owen points out that this change will mean every GTK app that calls net_wm_supports will wake up whenever metacity fetches a timestamp... which is even more context switches than the original bug ;-) The more robust fix might be to just create a window in MetaDisplay specifically for timestamp-generating purposes and don't use it for anything else.
Should probably see if anything other than timestamp-getting relies on that PropertyChangeMask, as well.
ah hmm, right. probably better if metacity uses a dedicated window for timestamp roundtrips
Created attachment 85605 [details] [review] Use a dedicated window for timestamp pinging This one works with Matthias' patch and doesn't need the PropertyChangeMask selection.
I committed the metacity patch to HEAD, and a dedicated-timestamp-window patch to the gnome-2-18 branch (for anyone that wants to use gtk+ HEAD with the stable branch of metacity). I've also opened bug 424763 about the excessive server pinging. Matthias: Do you want to commit your gtk+ patch, or would you like me too? I'll go ahead and mark this as fixed, since the metacity side is. :)
I you would go ahead and commit my version of the GTK+ patch to trunk, that would be great (I'm about to vanish for the rest of the day). Thanks for all the work, Elijah.
There's still a "booby trap" outstanding here right - we still know that gdk is going to mess with the event mask on leader_window, so the XSelectInput in create_offscreen_window is going to get messed up ... basically either that XSelectInput is not needed, or there is some kind of bug because it is needed and gdk messes it up. Possible fixes might be: - add a comment so the bug can be found more quickly later - remove the XSelectInput on leader_window if it isn't needed, so any bug in this area would always happen instead of only if wm_supports() is called - use a dedicated window for the WM_CHECK window - ... ?
Created attachment 85615 [details] [review] Clean up event mask handling and meta_create_offscreen_window Oh, right, that was kind of sloppy of me to not check. We were expecting PropertyNotify events on the leader window in session.c, although that was actually just a case of code duplication of meta_display_get_current_time_roundtrip. So I fixed that and removed the trap. It turns out there is another convenient one-time use of getting a property notify on the leader window, but it's just 25 lines down and so gdk won't have a chance to modify anything; I left it in and added a comment about it). I also cleaned up the event mask handling with meta_create_offscreen_window in general. Anything else you can spot in the patch Havoc? If not, I'll go ahead and commit.
Created attachment 85616 [details] [review] gah, here's the correct cleanup patch; ignore the last one
Looks good to me!
Okay, committed the cleanups and the gtk+ patch. Thanks Matthias, Havoc, and Owen!