Closed Bug 614416 Opened 14 years ago Closed 14 years ago

Building lists is too slow

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- final+

People

(Reporter: mossop, Assigned: mossop)

References

Details

(Keywords: perf)

Attachments

(1 file)

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: nobody → dtownsend
blocking2.0: --- → final+
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.
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.
Attached patch patch rev 1 (deleted) — Splinter Review
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)
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+
Whiteboard: [has patch][needs review Unfocused] → [has patch]
(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
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla2.0b8
Backed out due to test failures
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
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
Depends on: 1499222
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: