Closed Bug 392807 Opened 17 years ago Closed 17 years ago

Severe performance degradation loading file listings

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: bzbarsky, Assigned: dao)

References

Details

(Keywords: perf, regression)

Attachments

(3 files, 1 obsolete file)

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?
(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.
> 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...
Attached patch ditch auto-sorting (obsolete) (deleted) — Splinter Review
Assignee: nobody → dao
Status: NEW → ASSIGNED
Attachment #278399 - Flags: superreview?(bzbarsky)
Attachment #278399 - Flags: review?(bzbarsky)
Attached patch ditch auto-sorting (deleted) — Splinter Review
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)
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+
Keywords: checkin-needed
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
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.
(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?
Attachment #278780 - Flags: superreview?(bzbarsky)
Attachment #278780 - Flags: review?(bzbarsky)
Comment on attachment 278402 [details] [diff] [review] ditch auto-sorting Let's wait with this one.
Attachment #278402 - Attachment is obsolete: true
Keywords: checkin-needed
Attached patch try to load less icons (deleted) — Splinter Review
Attachment #278792 - Flags: superreview?(bzbarsky)
Attachment #278792 - Flags: review?(bzbarsky)
only specifying the extension will lead to different icons in some cases, compared to specifying the entire file url...
> 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.
> 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.
(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.
on windows .exe files; on linux files without extension, perhaps also image files (not sure about those). there are probably more cases.
I'm just saying we need the first patch for this to be bearable over here....
(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.
Keywords: checkin-needed
Attachment #278402 - Attachment is obsolete: false
(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.
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+
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+
Target Milestone: --- → mozilla1.9 M8
Flags: blocking1.9? → blocking1.9+
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?
All patches need to be checked in. Do you want me to combine them?
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(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.
(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.
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.
Depends on: 398414
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: