GNOME Bugzilla – Bug 596119
Search optimizations
Last modified: 2009-09-25 16:17:11 UTC
I haven't actually dug in with sysprof yet though my guess is what's slow is creating hundreds of actors we aren't actually showing. The first patch here is admittedly a hack for that.
Created attachment 143829 [details] [review] [genericDisplay] Limit search results display to 20 Kind of a fixed limit, but creating more than this is generally not useful in the inline display. If we revisit search+browse it will make sense to have more hits though.
Created attachment 143830 [details] [review] [genericDisplay] Optimize subsearches This is probably not the biggest optimization that needs to be made at least for application searching, but we can optimize the case where we're going from a search of "fi" to "fire" by just re-searching the list of things we already had that matched "fi" instead of looping over everything.
Created attachment 143853 [details] profiel Gjs has a builtin profiler in case anyone didn't know. This is just JS level profiling. I gathered this data by entering the overview once, then typing "fire" maybe 10 times, sometimes clearing with escape and sometimes backspacing. So what we can see here is that basically we spend all our time in genericDisplay.js:597, which is "_redisplay", as expected. This itself is mostly "_displayMatchedItems", which itself is mostly "_addDisplayItem". So I think we've confirmed here that what the doctor ordered is not to create and destroy tons of actors while searching.
Couldn't we wait for the user to stop typing before we start the search? At least on my box, I'm always stuck with the results for the first(s) char(s) of the word I'm typing, and then I need to wait for a while until the final result are shown. Pretty annoying. Maybe with a short delay of, say, 0.25s, we could avoid useless searches for most cases without losing usability.
(In reply to comment #4) > Couldn't we wait for the user to stop typing before we start the search? We want more or less immediate feedback when you start typing, I think. However, this doesn't imply doing a search on the first keystroke. Actually, it turns out we had a bug where we'd show *old* search results immediately, and then when the 150 millisecond timeout kicked in we'd do the real search. My first patch here fixes that. > At > least on my box, I'm always stuck with the results for the first(s) char(s) of > the word I'm typing, and then I need to wait for a while until the final result > are shown. Pretty annoying. Maybe with a short delay of, say, 0.25s, we could > avoid useless searches for most cases without losing usability. Barring say 1000+ recent documents or having an "Everything" install with 1000+ applications, there's no reason search shouldn't be effectively instant, IMO.
Created attachment 143949 [details] [review] Optimize subsearches This is probably not the biggest optimization that needs to be made at least for application searching, but we can optimize the case where we're going from a search of "fi" to "fire" by just re-searching the list of things we already had that matched "fi" instead of looping over everything.
Created attachment 143950 [details] [review] [ShellOverflowList] Only use visible actors for paging/allocation It's expected that containers skip not-visible actors when allocating, etc. https://bugzilla.gnome.org/show_bug.cgi?id=569119
Created attachment 143951 [details] [review] [Dash] Don't show search results sections until we've actually searched We queue a 150ms timeout when the user starts typing to avoid searching for the first keystroke. However, this caused us to change to the search mode, but show the leftover state of the search displays from an earlier search state. Instead, just hide the results sections until we've actually performed the current search once. https://bugzilla.gnome.org/show_bug.cgi?id=569119
Created attachment 143952 [details] [review] Optimize searching further Rather than destroying and re-creating actors during searching, instead just hide them. https://bugzilla.gnome.org/show_bug.cgi?id=569119
Created attachment 143953 [details] timings for searches Manually instrumenting the search timeout, you can see that we still have a largish initial hit for searches, but that on average we are twice as fast with the patches up till now.
Created attachment 143954 [details] new JS profile From this new profile data, you can see we spend most of our time in native code, so most of the really bad stuff from the JS side has been squeezed out. The biggest JS chunks are doing the initial loads of the data, after that comes "_getSearchMatchedItems" which is what's matching the items, so not too surprising.
So 100 foot view: search went from "very irritating" to "mostly acceptable" on my netbook with these patches; the initial search hit is the worst part. I think this is about as far as I'd go without significant code churn. What I think search should be is basically: Have a ShellGenericContainer which knows it has 3 sections. When its setSearch() is called, it asks each data source for results, in order. It then figures out which sections to display, and creates actors for as many results as it knows it can fit. When setSearch() is called after that it does the filtering on the current set of things. This way we avoid all of the ShellOverflowList/GenericDisplay/paging/etc. overhead. Also, we should move the searching into C; so ShellAppSystem has a GSList * get_matching_applications (ShellAppSystem *self, const char *search); Have a ShellRecentManager which also has get_matching_docs (ShellRecentManager *self, const char *search); etc. We should keep a cache of the g_utf8_casefold strings rather than re-lowercasing everything each time.
Results of some sysprof profiling come down to roughly: 20% painting (_clutter_clock_dispatch) 20% reallocation (big_box_allocate, clutter_group_allocate) 20% JS/Gjs bits (mix of pure JS, Gjs binding) 40% other (picking, sysprof itself, kernel time...) I think the stuff to cut out here is going to be the 20% reallocation and 20% JS bits.
Comment on attachment 143949 [details] [review] Optimize subsearches This looks good, other than this junk blank line: >@@ -488,6 +498,7 @@ GenericDisplay.prototype = { > this._removeAllDisplayItems(); > for (let i = 0; i < this._matchedItems.length; i++) { > this._addDisplayItem(this._matchedItems[i]); >+ > }
Comment on attachment 143950 [details] [review] [ShellOverflowList] Only use visible actors for paging/allocation >It's expected that containers skip not-visible actors when >allocating, etc. > >https://bugzilla.gnome.org/show_bug.cgi?id=569119 > >https://bugzilla.gnome.org/show_bug.cgi?id=596119 double URL on several of these; the first one is wrong The rest of the patch looks right.
Comment on attachment 143951 [details] [review] [Dash] Don't show search results sections until we've actually searched I'm not familiar with the search code, but this all looks right. >+ this._searchPending = this._searchActive && !searchPreviouslyActive; But the naming isn't totally intuitive to me, since it can be both "active" and "pending" at the same time? (Fix it if you have a better idea, commit it if not...)
Attachment 143949 [details] pushed as 32ef951 - Optimize subsearches Attachment 143952 [details] pushed as 159081d - Optimize searching further