GNOME Bugzilla – Bug 645313
improve search time
Last modified: 2011-03-21 23:52:47 UTC
Created attachment 183848 [details] [review] searchDisplay: don't create useless SearchResult's average constructing time for 1 SearchResult is 3 ms. It give > 400 result on query like 'a' (on my machine).
test: query | without patch | with patch 'a' | 2.9 sec | 0.5 sec 'e' | 2.2 sec | 0.45 sec
*** Bug 645334 has been marked as a duplicate of this bug. ***
*** Bug 645363 has been marked as a duplicate of this bug. ***
Just tested this, amazing performance improvement (even on queries with less < 400 results) This single patch has improved my shell experience considerably.
Comment on attachment 183848 [details] [review] searchDisplay: don't create useless SearchResult's >+ this._notDisplayedResult = results.slice(0); >+ this._terms = terms.slice(0); Is it actually necessary to copy the lists? It looks like nothing ever mutates them... If so, please at least add a comment "// copy the lists", since the ".slice(0)" idiom isn't immediately obvious. either way, good to commit
Review of attachment 183848 [details] [review]: I agree we have a big search performance problem right now, but less certain about this patch; see my comment below. What if we changed iconGrid.js to create actors lazily? Basically we do: this._grid.addItemPromise(Lang.bind(this, function () { return new SearchResult(this.provider, meta, terms); })); Then the grid consumes them as needed. So inside icongrid.js, we'd have a _lazyChildren variable which would be a list of functions. The tricky thing would be - I'm not sure if clutter is going to be happy with us calling _add_actor() from inside allocate. Maybe we queue it to a timeout? ::: js/ui/iconGrid.js @@ -284,2 @@ _computeLayout: function (forWidth) { - let children = this._grid.get_children(); This is an independent fix; let's commit it now. ::: js/ui/searchDisplay.js @@ +111,3 @@ + this.actor.connect('notify::width', Lang.bind(this, function() { + this._width = this.actor.width; + this._tryAddResults(); I don't like the recursion here; the first time we get allocated, this is going to fire, and then we potentially add something to a child, which in turn will cause a width notification.
Created attachment 184013 [details] [review] iconGrid: remove unused variable in _computeLayout
Created attachment 184014 [details] [review] searchDisplay: don't create useless SearchResult's > Is it actually necessary to copy the lists? yes > What if we changed iconGrid.js to create actors lazily? It will be used only here. > I'm not sure if clutter is going to be happy with us calling _add_actor() from inside allocate. It will print warning. > I don't like the recursion here; fixed
Created attachment 184020 [details] [review] searchDisplay: don't create useless SearchResult's (add missing import)
Review of attachment 184020 [details] [review]: It's not beautiful but the only alternative I had of changing icongrid.js isn't substantially better. Let's go with it then.