Closed
Bug 614416
Opened 14 years ago
Closed 14 years ago
Building lists is too slow
Categories
(Toolkit :: Add-ons Manager, defect)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla2.0b8
Tracking | Status | |
---|---|---|
blocking2.0 | --- | final+ |
People
(Reporter: mossop, Assigned: mossop)
References
Details
(Keywords: perf)
Attachments
(1 file)
(deleted),
patch
|
Unfocused
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
Assignee: nobody → dtownsend
blocking2.0: --- → final+
Assignee | ||
Comment 1•14 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.
Assignee | ||
Comment 2•14 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•14 years ago
|
||
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•14 years ago
|
Whiteboard: [has patch][needs review Unfocused]
Comment 4•14 years ago
|
||
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•14 years ago
|
Whiteboard: [has patch][needs review Unfocused] → [has patch]
Assignee | ||
Comment 5•14 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•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → mozilla2.0b8
Assignee | ||
Comment 7•14 years ago
|
||
Backed out due to test failures
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 8•14 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
Comment 9•14 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•