Closed
Bug 392807
Opened 17 years ago
Closed 17 years ago
Severe performance degradation loading file listings
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla1.9alpha8
People
(Reporter: bzbarsky, Assigned: dao)
References
Details
(Keywords: perf, regression)
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
bzbarsky
:
approval1.9+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
bzbarsky
:
approval1.9+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
bzbarsky
:
approval1.9+
|
Details | Diff | Splinter Review |
With the checkin for bug 294800, the time required to load the file listing for my home directory went from 4 seconds to 14 seconds (cold load; for a reload it went from 2 to 8 seconds). At the end of the load there is a fairly long freeze when the browser is completely unresponsive.
There's nothing particularly special about this directory other than having a few hundred files (most of them dotfiles).
Profile shows that we're spending about 21% of the time in layout, 50% running the DOMContentLoaded handler (this is what causes the freeze), 21% constructing frames, 5% starting image loads. Everything else is pretty minor.
About half or the frame construction time is style resolution, for what it's worth.
At the very least, would it be possible to presort the rows in C++ in the default sort order so we don't have to execute the JS to sort them? Failing that, can we do something about said JS effectively throwing away all layout work done to that point and redoing it? Perhaps when script is enabled we should default the body to display:none until we've sorted the rows? That's not as good as presorting in the c++, of course, since a lot of the time is the actual DOM mutation. But it's something.
Also, I'm seeing a good bit of relayout happening as the images go from alt text to image... I guess there's not much we can do about that.
Flags: blocking1.9?
Assignee | ||
Comment 1•17 years ago
|
||
(In reply to comment #0)
> At the very least, would it be possible to presort the rows in C++ in the
> default sort order so we don't have to execute the JS to sort them?
It's a stream, isn't it?
Maybe the script could move the rows as they appear in the document.
But the easiest fix would be to not automatically change the original sorting.
Reporter | ||
Comment 2•17 years ago
|
||
> It's a stream, isn't it?
Probably, yes... You've been in this code more recently than anyone else, though, so don't ask me. ;)
> But the easiest fix would be to not automatically change the original sorting.
That might be a reasonable approach, yeah...
Assignee | ||
Comment 3•17 years ago
|
||
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #278399 -
Flags: superreview?(bzbarsky)
Attachment #278399 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 4•17 years ago
|
||
Attachment #278399 -
Attachment is obsolete: true
Attachment #278402 -
Flags: superreview?(bzbarsky)
Attachment #278402 -
Flags: review?(bzbarsky)
Attachment #278399 -
Flags: superreview?(bzbarsky)
Attachment #278399 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 5•17 years ago
|
||
Comment on attachment 278402 [details] [diff] [review]
ditch auto-sorting
Looks good. r+sr+a=bzbarsky
Attachment #278402 -
Flags: superreview?(bzbarsky)
Attachment #278402 -
Flags: superreview+
Attachment #278402 -
Flags: review?(bzbarsky)
Attachment #278402 -
Flags: review+
Attachment #278402 -
Flags: approval1.9+
Assignee | ||
Updated•17 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 6•17 years ago
|
||
It would be good if the sorting and reordering could be sped up too. Unfortunately I don't have a concrete idea how to do this, other than using a XUL tree, which wouldn't be a trivial move at this point.
OS: Linux → All
Hardware: PC → All
Reporter | ||
Comment 7•17 years ago
|
||
One approach that might speed up reordering is to, instead of appending the rows to the table one at a time, append them all in the right order to a DocumentFragment and then append the DocumentFragment. That _might_ reduce the amount of time spent in frame construction.
Note also that even with the sorting disabled by default (by that patch) this is still about 2x slower than it used to be. At least in part this seems to be due to the way the icons are done: since all the URIs are different, we always miss the image cache, even in cases when we'd end up with the same icon. As a result, we end up having to load a _lot_ of "different" images.
Assignee | ||
Comment 8•17 years ago
|
||
(In reply to comment #7)
> One approach that might speed up reordering is to, instead of appending the
> rows to the table one at a time, append them all in the right order to a
> DocumentFragment and then append the DocumentFragment. That _might_ reduce the
> amount of time spent in frame construction.
That's indeed faster. Actually, it appears to be A LOT faster under certain conditions. I'll attach a second patch.
> Note also that even with the sorting disabled by default (by that patch) this
> is still about 2x slower than it used to be. At least in part this seems to be
> due to the way the icons are done: since all the URIs are different, we always
> miss the image cache, even in cases when we'd end up with the same icon. As a
> result, we end up having to load a _lot_ of "different" images.
Ok, 'moz-icon:.zip' then?
Assignee | ||
Comment 9•17 years ago
|
||
Attachment #278780 -
Flags: superreview?(bzbarsky)
Attachment #278780 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 10•17 years ago
|
||
Comment on attachment 278402 [details] [diff] [review]
ditch auto-sorting
Let's wait with this one.
Attachment #278402 -
Attachment is obsolete: true
Assignee | ||
Updated•17 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 11•17 years ago
|
||
Attachment #278792 -
Flags: superreview?(bzbarsky)
Attachment #278792 -
Flags: review?(bzbarsky)
Comment 12•17 years ago
|
||
only specifying the extension will lead to different icons in some cases, compared to specifying the entire file url...
Reporter | ||
Comment 13•17 years ago
|
||
> spend less time constructing frames
This about halves the time spent on the sorting here. So it still leaves total pageload about 3x slower than it used to be, with a multi-second complete freeze.
For the other patch, biesi's right. Let me try what impact the patch has, though.
Reporter | ||
Comment 14•17 years ago
|
||
> try to load less icons
This seems to help some with the initial load (as expected), but I'm getting a _lot_ of noise today for some reason, so hard to tell...
For the sorting, I think we should both not sort by default and do the sorting speedup in question.
Assignee | ||
Comment 15•17 years ago
|
||
(In reply to comment #12)
> only specifying the extension will lead to different icons in some cases,
> compared to specifying the entire file url...
Which cases would that be? I can imagine that chained extensions (.tar.gz) could make a difference, but are there more weird cases?
(In reply to comment #14)
> For the sorting, I think we should both not sort by default and do the sorting
> speedup in question.
Yes, as I wrote, I'd just wait with the first patch -- not saying it shouldn't go in at all.
Comment 16•17 years ago
|
||
on windows .exe files; on linux files without extension, perhaps also image files (not sure about those). there are probably more cases.
Reporter | ||
Comment 17•17 years ago
|
||
I'm just saying we need the first patch for this to be bearable over here....
Assignee | ||
Comment 18•17 years ago
|
||
(In reply to comment #16)
> on windows .exe files; on linux files without extension, perhaps also image
> files (not sure about those). there are probably more cases.
You would get generic icons in those cases. That wouldn't be a regression though, as the entire addresses aren't currently used. I think as a trade-off, that's acceptable.
Assignee | ||
Updated•17 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•17 years ago
|
Attachment #278402 -
Attachment is obsolete: false
Comment 19•17 years ago
|
||
(In reply to comment #18)
> You would get generic icons in those cases. That wouldn't be a regression
> though, as the entire addresses aren't currently used. I think as a trade-off,
> that's acceptable.
Well yes, it wouldn't be a regression. It would still be worse than what we could do though.
Reporter | ||
Comment 20•17 years ago
|
||
Comment on attachment 278780 [details] [diff] [review]
spend less time constructing frames
Looks good.
Attachment #278780 -
Flags: superreview?(bzbarsky)
Attachment #278780 -
Flags: superreview+
Attachment #278780 -
Flags: review?(bzbarsky)
Attachment #278780 -
Flags: review+
Attachment #278780 -
Flags: approval1.9+
Reporter | ||
Comment 21•17 years ago
|
||
Comment on attachment 278792 [details] [diff] [review]
try to load less icons
Let's try it.
Attachment #278792 -
Flags: superreview?(bzbarsky)
Attachment #278792 -
Flags: superreview+
Attachment #278792 -
Flags: review?(bzbarsky)
Attachment #278792 -
Flags: review+
Attachment #278792 -
Flags: approval1.9+
Updated•17 years ago
|
Target Milestone: --- → mozilla1.9 M8
Updated•17 years ago
|
Flags: blocking1.9? → blocking1.9+
Comment 22•17 years ago
|
||
it's not clear to me from skimming this bug what patches need to be checked in and what patches don't. Could you clarify?
Assignee | ||
Comment 23•17 years ago
|
||
All patches need to be checked in. Do you want me to combine them?
Comment 24•17 years ago
|
||
No, they applied on top of each other just fine.
Checking in nsIndexedToHTML.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp,v <-- nsIndexedToHTML.cpp
new revision: 1.84; previous revision: 1.83
done
Comment 25•17 years ago
|
||
(In reply to comment #6)
>It would be good if the sorting and reordering could be sped up too.
>Unfortunately I don't have a concrete idea how to do this, other than using a
>XUL tree, which wouldn't be a trivial move at this point.
We already have this. Start SeaMonkey, open Preferences, Debug, Networking, then select XUL tree-based directory listing.
Assignee | ||
Comment 26•17 years ago
|
||
(In reply to comment #25)
> We already have this. Start SeaMonkey, open Preferences, Debug, Networking,
> then select XUL tree-based directory listing.
That's a tree, literally; what I meant is just the tree element. The old HTML listing was more or less okay in that it was a flat table, not a tree. I don't think we want to change that.
Reporter | ||
Comment 27•17 years ago
|
||
So... since this got marked fixed (instead of filing and fixing bugs on particular perf issues until this was actually fixed), we need a followup bug on the remaining regression.
You need to log in
before you can comment on or make changes to this bug.
Description
•