GNOME Bugzilla – Bug 704912
Cache search result display actors
Last modified: 2013-10-30 17:02:15 UTC
While investigating search performance, I found that we were constantly requesting icons from our remote search providers, and that constantly deserializing search result icons in the main thread and the large amount of DBus traffic per-character were the source of most of our troubles. This attempts to cache result display actors when they're first created so that if we see the same result ID on future searches we don't re-fetch the actual display results.
Created attachment 250169 [details] [review] overviewControls: Show the workspace thumbails when dragging search results The workspace thumbnails were accidentally hidden.
Created attachment 250170 [details] [review] search: Actually crash when seeing errors from a native search provider We don't implement many of these, and not catching the error lets us see stack traces and other such information a lot faster.
Created attachment 250171 [details] [review] appDisplay: Use a proper string key for the app search provider Since we're going to be caching results based on the result ID, we need to return a string-based result ID to cache on.
Created attachment 250172 [details] [review] searchDisplay: Remove dead code The only GridSearchResult left is app results, which is guaranteed to have a dragActivateResult.
Created attachment 250173 [details] [review] searchDisplay: Make the search result actors stateless, by removing terms We want to cache result actors between searches, so we shouldn't instantiate them with search-specific info.
Created attachment 250174 [details] [review] searchDisplay: Cache result display actors When we create a result actor, cache it, so it can be used for subsearches of the same initial. For now, to keep memory usage and the stage graph relatively clean, don't persist the actors across searches, but maybe we should do this in the future. This also means that we don't query getResultMetas for items that we've seen in the same initial search.
Review of attachment 250169 [details] [review]: OK.
Review of attachment 250170 [details] [review]: Tbh I fail to see the point of this ...
(In reply to comment #1) > Created an attachment (id=250169) [details] [review] > overviewControls: Show the workspace thumbails when dragging search results > > The workspace thumbnails were accidentally hidden. That's not true, it was an intentional change. See bug 686984 (and commit 1db6d15677221e in particular).
(In reply to comment #8) > Tbh I fail to see the point of this ... When hacking on some of these patches, I got an error like: "A Error has occured in applications: Expected object for 'List', got string" or something like that. I spent a lot of time grepping for "A Error", before finally trying "occured" and managing to find the source of the error. Having a stack trace would have been a lot more useful out of the gate.
(In reply to comment #10) > (In reply to comment #8) > > Tbh I fail to see the point of this ... > > When hacking on some of these patches, I got an error like: > > "A Error has occured in applications: Expected object for 'List', got string" > > or something like that. I spent a lot of time grepping for "A Error", before > finally trying "occured" and managing to find the source of the error. Having a > stack trace would have been a lot more useful out of the gate. Easier for you as a developer sure ... for a user it is just annoying. How often does this happen? Lets not repeat the "widget not on stage" mistake.
Review of attachment 250169 [details] [review]: Needs design ack first (see Florian's comment).
(In reply to comment #11) > Easier for you as a developer sure ... for a user it is just annoying. How > often does this happen? Lets not repeat the "widget not on stage" mistake. It happens whenever the search provider code is broken. I'm fine with leaving it out.
Created attachment 250245 [details] [review] searchDisplay: Remove dead code We cannot drag search results to the workspace thumbnails anymore, so remove the shellWorkspaceLaunch implementation. OK, then. If it's intentional, then let's clean up some of the leftover code.
Review of attachment 250245 [details] [review]: You can't still drag to the workspace (on secondary monitors), so the code in workspace.js needs to be updated. Also appDisplay has shellWorkspaceLaunch too, you probably want to clean up that.
(In reply to comment #15) > Review of attachment 250245 [details] [review]: > > You can't still drag to the workspace (on secondary monitors), so the code in > workspace.js needs to be updated. > Also appDisplay has shellWorkspaceLaunch too, you probably want to clean up > that. And I mean you *can* still drag etc.
Are you waiting on design for this?
(In reply to comment #17) > Are you waiting on design for this? Uhm, I don't think so, the dubious patch was just rejected.
The dead code removal was something extraneous I noticed; it doesn't really matter for the rest of the series to go in from what I remember.
Review of attachment 250171 [details] [review]: ::: src/shell-app-system.c @@ +635,3 @@ + matches = g_slist_prepend (matches, (char *) shell_app_get_id (SHELL_APP (l->data))); + + return g_slist_reverse (matches); Are you changing memory management here? Previously the new list would contain the same nodes as the old ones, now it's a different list, so the old ones need to freed separately.
Review of attachment 250173 [details] [review]: Ok
Review of attachment 250174 [details] [review]: This looks good, except that it has problems with providers uncapable of generating persistent search ids or meta ids. This is already a problem, in the sense that if the meta id is not stable, after one minute the results are not activatable any more, but this patch makes it worse by extending the issue to result ids as well.
Attachment 250171 [details] pushed as 27cac10 - appDisplay: Use a proper string key for the app search provider Attachment 250173 [details] pushed as 3749b09 - searchDisplay: Make the search result actors stateless, by removing terms Attachment 250174 [details] pushed as af06b78 - searchDisplay: Cache result display actors