GNOME Bugzilla – Bug 597120
window::user-time connection disconnection
Last modified: 2009-10-06 16:56:45 UTC
I'm seeing pretty awful performance typing in the shell right now - I'm not sure exactly all of what's going on, but a quick approach of attaching to the shell and hitting control-c revealed one bad thing that is happening. As I type in an application window that I previously selected from a window menu (from altTab), every key stroke is triggering a Javascript callback from notify::user-time. The only such connection is: for (let i = 0; i < this.windows.length; i++) { this.windows[i].connect('notify::user-time', Lang.bind(this, this._resortWindows)); } in appIcon.js. Since the signal id isn't being assigned anywhere I don't see how this could ever be cleaned up, and since the signal connection references 'this' the menu isn't going to get freed. (There's probably a whole lot of menus getting notified in fact?)
on this
Created attachment 144622 [details] [review] [AppIcon] Disconnect from window user time notifications, only sort when mapped Before we were badly leaking AppIcons by not disconnecting from the window signal handlers when our actor got destroyed. This caused us to repeatedly re-sort the windows for each AppIcon that had ever been displayed with obvious bad consequences. Besides simply chaining the signals to the lifetime of the AppIcon actor, we also only do the sorting if we're mapped. This decreases the amount of work to do in the not-mapped case.
Review of attachment 144622 [details] [review]: This seems very complicated to me. Why not just get the windows and sort them when popping the menu up? Yes, you are already calling AppIcon.get_windows_for_app() to (for the glow case) figure out how many glows to show, but the cost of doing that again when popping up the menu can't be as much as from the complexity here. (And might as well just destroy the menu when popping it down ... it has to be fast enough the *first* time. Having it pop up faster when activated a second time doesn't help any.)
(In reply to comment #3) > Review of attachment 144622 [details] [review]: > > This seems very complicated to me. Why not just get the windows and sort them > when popping the menu up? Two reasons; first we need the window order to know what window to activate at least. I suppose we could just sort once when we load the icon, but: Second, In the future I'd like AppIcon to really work dynamically, as part of what we need to avoid recreating the whole AppWell on changes.
Created attachment 144841 [details] [review] [AppIcon] Add accessor for window lists: getWindows(), getCachedWindows() Rather than by default holding onto the window list, add two separate functions, one which always returns an updated window list, and one which returns the window list as known from the first time getCachedWindows() is called, typically used when the appIcon is created. Calling getWindows() will explicitly update the cache if one exists.
Comment on attachment 144841 [details] [review] [AppIcon] Add accessor for window lists: getWindows(), getCachedWindows() Hmm, hadn't noticed the usage of windows in other files. We want to be very conservative about breaking stuff in altTab.js at the moment, because there's enough of a struggle going on with changes there at the moment as is. The getWindows() vs. getCachedWindows() thing is clearly Not OK. I don't think the from-scratch code computation of: - Iterate through all windows on the desktop - Find the ones associated with the app - Look up the JS proxy objects for them - Create an Array and add them to the array - Sort that array of 3-4 items Is really impossible to just do every time even when need the list of windows. But trying to keep altTab.js changes to a minimum would encourage not having a getWindows() accessor. I'll take a detailed look at your other patch.
Review of attachment 144622 [details] [review]: Looks OK for now. I think it's fundamentally wrong to have the appIcon be the "model" for the application state - the appIcon should just be a UI elemen. If the ShellAppMonitor isn't good enough for model object: - It's a pain to extend the API - Because the API is not OO - based on appIds, not application objects - Because we're worried about overhead in creating JS objects when we call list-valued accessor objects Then we should create a set of JS model objects to shadow it. But that would require more extensive changes. I'm not really sure that putting worked into notify::mapped for this is a good idea. Entering the overlap is the place where we're most obviously slow at the moment; I'd rather just have as little state as possible in the overview - to me the overview doesn't need state for this. But also OK for now.