Building lists is too slow

VERIFIED FIXED in mozilla2.0b8

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

({perf})

Trunk
mozilla2.0b8
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(1 attachment)

(Assignee)

Description

7 years ago
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.
(Assignee)

Updated

7 years ago
Assignee: nobody → dtownsend
blocking2.0: --- → final+
(Assignee)

Comment 1

7 years ago
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.
Keywords: perf
(Assignee)

Comment 2

7 years ago
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.
(Assignee)

Comment 3

7 years ago
Created attachment 493144 [details] [diff] [review]
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.
Attachment #493144 - Flags: review?(bmcbride)
(Assignee)

Updated

7 years ago
Whiteboard: [has patch][needs review Unfocused]
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?
Attachment #493144 - Flags: review?(bmcbride) → review+
(Assignee)

Updated

7 years ago
Whiteboard: [has patch][needs review Unfocused] → [has patch]
(Assignee)

Comment 5

7 years ago
(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
(Assignee)

Comment 6

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/6ad26f209603
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla2.0b8
(Assignee)

Comment 7

7 years ago
Backed out due to test failures
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 8

7 years ago
Re-landed: http://hg.mozilla.org/mozilla-central/rev/61deb05747d6
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Depends on: 616841
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.
Status: RESOLVED → VERIFIED
Depends on: 640530
You need to log in before you can comment on or make changes to this bug.