614416 - Building lists is too slow

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

Been noticing this for a while, switching between the lists seems to take much longer than it should. Timing on my machine is showing that the providers are responding fast enough, within a few ms in general but building and sorting the DOM is eating time (420ms for 8 plugins in my case!).

Probably a couple of causes for this but this needs to be better.

Comment 1

[Dave Townsend [:mossop]]

Setting the size attribute for plugins is eating up 110 ms for 8 plugins. We don't cache this info and dynamically look it up which on OSX is often a recursive directory scan of all files in the plugin's bundle. We could cache this but since we no longer offer sorting by size we just don't need to retrieve this at this point anyway.

Comment 2

[Dave Townsend [:mossop]]

It also turns out we set the size attribute twice for every item, for some reason the second time around uses even more time so with those two gone we drop down to 84ms to build the plugins list.

We can drop this down to around 40ms if we ditch the XUL sort service or stop it from re-running all the XBL gunk with every call.

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This stops populating the size attribute and switches us away from the XUL sort service, it was only slowing us down. It also fixes the main sorting test to actually test things (DOH!). With this I see vastly improved speeds switching to the plugins pane on OSX, about a 85% improvement. On other panes and other platforms we should see about a 50% improvement.

It still takes around 5ms per add-on to build the list for me, it might be nice to improve that more but this is a big win on its own.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 493144 [details] [diff] [review]
patch rev 1

Me like! I had been wondering about ditching nsIXULSortService just to gain some flexibility, so its nice to have a performance improvement because of that too. Do you know how much of the slowdown was the sort service itself, rather than just having to run the XBL constructors twice?

>+  aElements.sort(function(a, b) {
>+    function getValue(aObj) {
>+      if (!aObj)
>+        return null;
>+
>+      if (aObj.hasAttribute(aSortBy))
>+        return aObj.getAttribute(aSortBy);
>+
>+      addon = aObj.mAddon || aObj.mInstall;
>+      if (!addon)
>+        return null;
>+
>+      return addon[aSortBy];
>+    }

Can getValue() be moved out, so its just within sortElements(), like intCompare(), etc? Don't need to have that function (getValue) re-created every time .sort() calls the comparison function.


>+      var start = Date.now();

Leftover benchmarking code?

Comment 5

[Dave Townsend [:mossop]]

(In reply to comment #4)
> Comment on attachment 493144 [details] [diff] [review]
> patch rev 1
> 
> Me like! I had been wondering about ditching nsIXULSortService just to gain
> some flexibility, so its nice to have a performance improvement because of that
> too. Do you know how much of the slowdown was the sort service itself, rather
> than just having to run the XBL constructors twice?

It looked like the sort service was pretty cheap in terms of sorting, the only reason to drop it was that obviously it can't sort without having inserted first so we get that double cost. After that it didn't seem smart to use our own sort for the initial sort and then the XUL sort service for later sorts. This does make us a bit more flexible which is nice and the JS sort function is quick enough anyway.

It's possible that in some cases we could make resorting faster here, if we could identify a number of elements that are already in the right order for example and only move the others then we save the XBL costs for those elements, but that probably isn't worth exploring for now.

> >+      var start = Date.now();
> 
> Leftover benchmarking code?

Oops

Comment 6

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/6ad26f209603

Comment 7

[Dave Townsend [:mossop]]

Backed out due to test failures

Comment 8

[Dave Townsend [:mossop]]

Re-landed: http://hg.mozilla.org/mozilla-central/rev/61deb05747d6

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That's a really fantastic improvement. Compared the loading between b7 and latest nightly build, which clearly shows the speed-up. But sadly it has been regressed bug 616841.