GNOME Bugzilla – Bug 732290
Application totally freezes up
Last modified: 2014-09-24 16:36:53 UTC
Using version 3.12.2 The application totally freezes up several times a day. Pressing Wait at the dialog that comes up several times usually helps in the end and the app goes back to normal, but it usually takes quite a while.
Unfroze again after 5-6 minutes just now.
Stacktrace welcome.
How do I create that?
In theory, https://wiki.gnome.org/Community/GettingInTouch/Bugzilla/GettingTraces/Details should cover running it under gdb (though after the application has frozen, you will need to press Ctrl+C once in gdb, and then on the prompt enter "thread apply all bt")
(In reply to comment #4) > In theory, > https://wiki.gnome.org/Community/GettingInTouch/Bugzilla/GettingTraces/Details > should cover running it under gdb Probably the easiest way to run polari under gdb is starting it as POLARI_RUN_DEBUG=1 polari
http://pastebin.com/fDFpfbHC
Looking into this a bit, I found that it is due to having plenty of widgets around - I found polari with ~20 channels creates around 25k widgets. And each widgets' style context connects to the -gtk-private-changed signal of the singleton style cascade object - this exposes scalability issues of gobject signals. I've filed bug 737009 with a proposed fix.
It might be worth considering to not keep all the user lists for all the channels around in widget form - thats why there are > 20k widgets here.
Created attachment 286711 [details] [review] userList: Don't keep userList widgets of inactive rooms Since commit 9ab1ab5f09e3, we only keep the user list of the active room in the widget hierarchy, which helped performance quite a bit. However GTK+ still does a significant amount of work for unparented widgets on style changes, so it is actually cheaper to destroy and recreate the user list widget each time the active room changes. Kudos to Matthias Clasen for the debugging legwork.
Created attachment 286712 [details] [review] userList: Only create user list widget when necessary We currently update the user list every time the active room changes. However the user may never actually pop up the list, in which case we are keeping a bunch of widgets around that are not shown at all - avoid unnecessary work by only creating the list when we are about to show it.
Created attachment 286713 [details] [review] userList: Only create user details on demand Just like the user list as a whole, a user's details may never be shown at all - in fact, for most users in the list this is the expected case. As the user details (for each user in the list!) contribute significantly to our excessive widget count, do the same as for the user list and only create the details widget when we are actually about to show it.
(In reply to comment #7) > Looking into this a bit, I found that it is due to having plenty of widgets > around - I found polari with ~20 channels creates around 25k widgets. And each > widgets' style context connects to the -gtk-private-changed signal of the > singleton style cascade object Thanks for looking into this! I admit that I was not aware that widgets that were outside the hierarchy (e.g. without a parent) were still involved in style changes. So yeah, your suggestion of not dragging around a large number of unnecessary widgets makes a whole lot of sense ...
Created attachment 286716 [details] [review] userList: Disconnect room signals on widget destruction Now that the user list lifecycle is no longer tied to the corresponding room, we need to disconnect the signals when destroying the widget.
These patch help greatly as far as the freezing is concerned - I see ~2000 widgets now, where I used to see 20000. One thing I find annoying though is that I now get a somewhat long 'Loading Details' spinner whenever I click on a user. It would be great to obtain that information ahead of time (without constructing widgets yet) or at least cache it once you've gotten it (so I don't have to see the spinner more than once per user.
(In reply to comment #14) > One thing I find annoying though is that I now get a somewhat long 'Loading > Details' spinner whenever I click on a user. Retrieving the details can take quite a while unfortunately, but that should be unrelated to these patches. > It would be great to obtain that information ahead of time (without > constructing widgets yet) or at least cache it once you've gotten it I've tried fetching all details in the past, it doesn't scale. We don't get change notifications, so I think we still want to re-fetch details when re-opening, but it should be easy to not show the spinner in that case and just update the existing details when the response comes in ...
One thing I still notice with these patches is that the number of signal handlers / widgets goes up over time, even when I'm not interacting with the application. Leak or lack of gc ?
Attachment 286711 [details] pushed as aa63ad6 - userList: Don't keep userList widgets of inactive rooms Attachment 286712 [details] pushed as 5d79def - userList: Only create user list widget when necessary Attachment 286713 [details] pushed as 04e168a - userList: Only create user details on demand Attachment 286716 [details] pushed as c102d3f - userList: Disconnect room signals on widget destruction Keeping open for now, at least for the widgets/signals issue Matthias mentioned.
Very nice patches, they clearly go in the right direction. Another source I've found of excessive widget creation is the tab completion list, it is actually maintained for every room you're connected to, and every change in the userlist destroys and creates again the widgets in the list, putting some pressure on the GC on populated channels (both because of size list and because userlist changes are more frequent there) I'm attaching a couple of additional patches to only have a completion list on the currently focused entry, and to make row widgets reused across changes.
Created attachment 286976 [details] [review] tabCompletion: Ensure row widgets are reused when rebuilding the list The previous approach of destroying all widgets and recreating the list from scratch wasn't too friendly with the GC, as quite a few widgets would be created and artificially kept alive longer on every change in the userlist of the tracked rooms.
Created attachment 286977 [details] [review] entryArea: Ensure only the focused chat entry has a completion list Otherwise tab completion lists are maintained for every room simultaneously, rocketing the amount of widgets as userlist changes happen in any of the active rooms.
Review of attachment 286976 [details] [review]: Looks good, thanks! ::: src/tabCompletion.js @@ +66,2 @@ for (let i = 0; i < completions.length; i++) { + let nick = completions[i].toLowerCase(); This will miss changes in nick capitalization, e.g. from mynick to myNick - either match on the actual nick, or make sure to update label/_text on the reused widget
Review of attachment 286977 [details] [review]: ::: src/entryArea.js @@ +81,3 @@ this.widget.add(this._entry); + this._entry.connect('focus-in-event', Lang.bind(this, Not sure about using focus-in/out - there are plenty of cases where the focus moves for other reasons than changing room (opening the user list, changing the nick, copying some text, ...). Unless I'm overlooking something, map/unmap or ChatroomManager::active-changed appear to be better choices.
Created attachment 286990 [details] [review] tabCompletion: Ensure row widgets are reused when rebuilding the list The previous approach of destroying all widgets and recreating the list from scratch wasn't too friendly with the GC, as quite a few widgets would be created and artificially kept alive longer on every change in the userlist of the tracked rooms.
Created attachment 286991 [details] [review] entryArea: Ensure only the mapped chat entry has a completion list Otherwise tab completion lists are maintained for every room simultaneously, rocketing the amount of widgets as userlist changes happen in any of the active rooms.
Review of attachment 286990 [details] [review]: ::: src/tabCompletion.js @@ +70,3 @@ + if (row) { + widgetMap[nick] = row; + this._list.remove(row); Nice! I prefer this over the previous patch version
Review of attachment 286991 [details] [review]: OK
Attachment 286990 [details] pushed as 0bdfbed - tabCompletion: Ensure row widgets are reused when rebuilding the list Attachment 286991 [details] pushed as 00aedc1 - entryArea: Ensure only the mapped chat entry has a completion list