GNOME Bugzilla – Bug 613398
Don't trap XErrors in meta_compositor_process_event
Last modified: 2010-04-07 11:29:31 UTC
Doing this allows apps to DoS the compositor and it seems not to be needed.
Created attachment 156614 [details] [review] Don't trap XErrors in meta_compositor_process_event meta_compositor_process_event uses meta_error_trap_push/pop for every event processed by meta_compositor_process_event which could result into fast drawing apps to DoS the compositor. The comment in the code suggested that those are added "just in case". Running without them seems to fix the DoS issues and not add any side effects. Doing a synchronous roundtrip for every event isn't a good idea from a performance POV anyway. Other compositors like compiz seems not to do this either.
I've did some timing when tracker-status-icon is started or not, with or without this patch : with mutter 2.29.1 and without tracker-status-icon started, time for gnome-shell to be visible with a full session : 13.7s with mutter 2.29.1 and with tracker-status-icon started, time for gnome-shell to be visible with a full session : 9m57s with mutter 2.29.1 + this patch + tracker-status-icon started, time for gnome-shell to be visible with a full session : 1m36s better but not perfect :)
with mutter 2.29.1 + patches from bug 611838 + tracker-status-icon started, time for gnome-shell to be visible with a full session : 16min with mutter 2.29.1 + patches from this bug and bug 611838 + tracker-status-icon started, time for gnome-shell to be visible with a full session : 2min29s
Created attachment 156982 [details] sysprof trace this is a sysprof trace with gnome-shell 2.29.1 + mutter 2.29.1 (unpatched)
Looked into the tracker-status-icon problem: * I was unable to track down *why* tracker-status-icon was continually setting its icon during shell startup. At some point it stopped happening for me. it's probably some bug or pathology in the trackerd idle code. * The fact that setting the status is queueing reallocations in the shell, is mostly due to missing optimizations in GtkImage - I filed bug 613833. * The fact that queueing reallocations is causing the shell to fail catastrophically is due to a clutter problem - that coincidentally I identified a few days ago as a theoretical problem from ereading the code. See http://bugzilla.openedhand.com/show_bug.cgi?id=2024#c2 - "but if there's lots of damage coming in and layout is rapidly happening we might never redraw and spend all our [time] reallocating." That's exactly what is happening here.
I guess it stopped happening to you because your tracker index was up to date, so tracker-status-icon stopped its animation..
the Clutter bug has been closed and the patch applied.
Created attachment 158067 [details] [review] Remove compositor-internal window lookup code I went through the code paths that were inside the error trap, and there was actually one place where an error trap was needed. But it was for a silly reason. So, instead of moving the error trap to around just the single call, this patch simplifies things and removes a bunch of unnecessary work.
Review of attachment 156614 [details] [review]: This patch looks good (when combined with the patch I just attached), but the commit message is far too verbose. :-) I think it's enough to say that the synchronous error trap is unnecessary and a performance problem.
Comment on attachment 158067 [details] [review] Remove compositor-internal window lookup code Looks good, thanks.
Attachment 156614 [details] pushed as f77507e - Don't trap XErrors in meta_compositor_process_event with a less verbose commit message.