Closed
Bug 82854
Opened 23 years ago
Closed 23 years ago
FilePicker: Loading a directory with large number of files is very slow
Categories
(SeaMonkey :: UI Design, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.7
People
(Reporter: jg, Assigned: bryner)
References
Details
(Keywords: perf, Whiteboard: [nav+perf])
Attachments
(1 file)
(deleted),
patch
|
bugs
:
superreview+
|
Details | Diff | Splinter Review |
This is a spinoff from bug 72482 which was a similar problem. Creating a new bug
here to focus on another specific issue.
The problem is that while navigating in the new Filepicker in reassonably-sized
directories is reasonably speedy, as soon as you hit one in which there are a
large number of files we get a big slowdown.
Steps to reproduce:
1. File->Open File
2. Navigate to /dev
Expected results:
Should load the directory in a fraction of a second.
Actual results:
Directory is displayed in five seconds.
Additional information:
The directory itself:
jg@cyberstorm:~$ ls -l /dev/ | wc -l
1583
Using X-Chat I can pull up that directory in it's GTK filepicker in (perceived)
0.1 seconds. It shows all the files fine.
Using Nav4.x the filepicker does not show the /dev files and thus cannot be
compared to.
Also, I have a directory with 3682 items that takes approximately fifteen
seconds to load in Mozilla, yet 4.x loads it in approximately 0.2 seconds (only
slightly slower than X-Chat, barely noticable).
Note that of course during this excessive loading time, the entire app is frozen.
My build is home-built, from the tip today. I use:
--disable-debug
--enable-optimize=O2
My hardware is a K6-2 300Mhz 256Mb RAM, 128Mb swap, and using kernel 2.4.4 I
have tons of memory buffered ready for use; certainly no lack of free memory.
Reporter | ||
Comment 1•23 years ago
|
||
Adding keywords.
I'm adding bryner, jag and hyatt as cc. Bryner did the filepicker and might be
able to tell what's causing the speed problem. Jag has also done some code here
and may be able to help. If the problem is outliner-related, then hyatt's our dude.
I don't believe this is a immediately-critical problem, but it could be a simple
fix and thus I'm nominating for 0.9.2 and 1.0.
Updated•23 years ago
|
Component: XP Miscellany → XP Apps: GUI Features
QA Contact: brendan → sairuh
Assignee | ||
Comment 3•23 years ago
|
||
I think this is about as fast as it's going to get without some work on
nsLocalFileUnix.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.3
Updated•23 years ago
|
Whiteboard: [nav+perf]
Reporter | ||
Comment 4•23 years ago
|
||
A quick dump() immediately before
http://lxr.mozilla.org/seamonkey/source/xpfe/components/filepicker/res/content/nsFileView.js#366
and found the following:
I went into a directory full of .jpg files. The load was dead slow, and the dump
statement certainly fired off many times.
What's interesting is that I changed the filter to show only html files (which
predictably came up real quickly), and then changed it back to Images.
On going back to images the dump didn't fire once, and the loading was
adequately fast. So whatever happens in that function (setDirectory()) is really
slowing us down.
Updated•23 years ago
|
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Assignee | ||
Comment 5•23 years ago
|
||
moving these out to 0.9.5 since it doesn't look like I will have time for 0.9.4.
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Comment 6•23 years ago
|
||
I've also seen this on Windows 2000. Last Build ID tested: 2001082803 still
performs bad on W2k. Mr. Green maybe you can set the OS Platform to ALL
Comment 7•23 years ago
|
||
This is a Linux bug. It is not a win32 bug. No morphing allowed.
Comment 8•23 years ago
|
||
Vincente, this bug is about the linux filepicker we wrote. On W2k we use the OS
filepicker. If it's slow, file a bug with Microsoft ;-)
Comment 9•23 years ago
|
||
Mass-moving lower-priority 0.9.5 bugs off to 0.9.6 to make way for remaining
0.9.4/eMojo bugs, and MachV planning, performance and feature work. If you
disagree with any of these targets, please let me know.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Assignee | ||
Comment 10•23 years ago
|
||
The C++ rewrite for the outliner view is now checked in (turned off). I'm
attaching a patch to turn it on.
Assignee | ||
Comment 11•23 years ago
|
||
Comment 12•23 years ago
|
||
Comment 13•23 years ago
|
||
In nsFileView.cpp:
133 mDirectoryAtom = NS_NewAtom("directory");
134 mFileAtom = NS_NewAtom("file");
This leaks. Use mDirectoryAtom = dont_AddRef(NS_NewAtom("directory"));
145 NS_INTERFACE_MAP_BEGIN(nsFileView)
146 NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIFileView)
147 NS_INTERFACE_MAP_ENTRY(nsIFileView)
148 NS_INTERFACE_MAP_ENTRY(nsIOutlinerView)
149 NS_INTERFACE_MAP_END
150
151 NS_IMPL_ADDREF(nsFileView)
152 NS_IMPL_RELEASE(nsFileView)
You can simplify this to:
NS_IMPL_ISUPPORTS2(nsFileView, nsIOutlinerView, nsIFileView);
179 if (aOnlyDirs == mDirectoryFilter)
180 return NS_OK;
You could put similar logic in SetShowHiddenFiles()
265 if (isDirectory) {
266 PRBool isHidden;
267 theFile->IsHidden(&isHidden);
268 if (mShowHiddenFiles || !isHidden) {
Why do we only hide directories? I don't quite recall.
301 for (chr = aFilterString; *chr; ++chr) {
306 // ; will be followed by a space, and then the next filter
307 chr += 2;
If someone passes in a string like "abc;", this won't deal all too cleanly with
it. Can this ever happen?
Also, this should be |++chr;|, I think:
"abc; def"
When you find the ';', you copy "abc", then make |chr| point at the 'd'. Then
the |for| loop increments |chr|, which now points at 'e'. Not too evil, I guess,
except you'll break if your input is ever something like "abc; ; def", since
|aPos| (which should really just be named |pos|) now points at the second ';'
and will never be fixed since |chr| never was able to check it.
328 if (mOutliner) {
329 mOutliner->RowCountChanged(dirCount, newFileRows - oldFileRows);
330
331 PRInt32 commonRange = PR_MIN(newFileRows, oldFileRows);
332 if (commonRange)
328 if (mOutliner) {
329 mOutliner->RowCountChanged(dirCount, newFileRows - oldFileRows);
330
331 PRInt32 commonRange = PR_MIN(newFileRows, oldFileRows);
332 if (commonRange)
333 mOutliner->InvalidateRange(dirCount, dirCount + commonRange);
334 }
I thought you said this did weird things when you tried that in |SetDirectory|?
Why does this work here, but not for |SetDirectory|?
347 if (0 <= currentIndex) {
|if (currentIndex >= 0)|, _please!_
351 nsCOMPtr<nsISupports> elem =
dont_AddRef(mDirList->ElementAt(currentIndex));
352 CallQueryInterface(elem, aFile);
I think you could do:
mDirList->QueryElementAt(currentIndex, NS_GET_IID(nsIFile), aFile));
and spare an AddRef and Release. Same thing a little lower.
356 if ((currentIndex - dirCount) < fileCount) {
Could that be simplified to |if (currentIndex < mTotalRows)|?
409 else if ((aRow - dirCount) < fileCount)
Could this be simplified to |else if (aRow < mTotalRows)|?
512 if (aRow < (PRInt32) dirCount) {
515 } else if ((aRow - dirCount) < fileCount) {
You should cast fileCount to PRInt32 too, then.
518 } else {
519 // invalid row
520 *aCellText = ToNewUnicode(NS_LITERAL_STRING(""));
521 return NS_OK;
522 }
Will this ever happen? If so, shouldn't you then also check that we're not only
displaying directories before returning the text on a file? In other words,
shouldn't it be:
515 } else if (aRow < mTotalRows) {
See also lines 356 and 409.
541 } else
542 *aCellText = ToNewUnicode(NS_LITERAL_STRING(""));
Will this ever happen?
622 for (PRUint32 i = 0; i < count; ++i) {
623 nsCOMPtr<nsIFile> aFile = do_QueryElementAt(mFileList, i);
just call this |file|, it's not an argument, so no need for the |a|. Also, I
believe it's faster to declare the nsCOMPtr outside the loop and reuse it.
632 for (PRInt32 i = 0; i < filterCount; ++i) {
You're hiding the outer |i| here (I'm sure the compiler warned about this).
Nothing wrong with that per se, except for those crappy compilers that don't do
scoping in |for| loops well, so you'll get a compile error for "redeclaring
variable" or some such.
737 nsCOMPtr<nsISupports> item = aArray->ElementAt(i);
738 CallQueryInterface(item, &(array[i]));
I'm still wondering if you could do:
aArray->QueryElementAt(i, NS_GET_IID(nsIFile), &(array[i]));
and spare an AddRef and Release. See also up somewhere near line 350.
Another day, another file.
Assignee | ||
Comment 14•23 years ago
|
||
>In nsFileView.cpp:
>
>133 mDirectoryAtom = NS_NewAtom("directory");
>134 mFileAtom = NS_NewAtom("file");
>
>This leaks. Use mDirectoryAtom = dont_AddRef(NS_NewAtom("directory"));
Fixed.
>145 NS_INTERFACE_MAP_BEGIN(nsFileView)
>146 NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIFileView)
>147 NS_INTERFACE_MAP_ENTRY(nsIFileView)
>148 NS_INTERFACE_MAP_ENTRY(nsIOutlinerView)
>149 NS_INTERFACE_MAP_END
>150
>151 NS_IMPL_ADDREF(nsFileView)
>152 NS_IMPL_RELEASE(nsFileView)
>
>You can simplify this to:
>
>NS_IMPL_ISUPPORTS2(nsFileView, nsIOutlinerView, nsIFileView);
Fixed.
>179 if (aOnlyDirs == mDirectoryFilter)
>180 return NS_OK;
>
>You could put similar logic in SetShowHiddenFiles()
Fixed.
>265 if (isDirectory) {
>266 PRBool isHidden;
>267 theFile->IsHidden(&isHidden);
>268 if (mShowHiddenFiles || !isHidden) {
>
>Why do we only hide directories? I don't quite recall.
It's actually an issue of the "show hidden files" pref being applied to files
and directories at different times. For files, we apply the hidden-ness as part
of the filter, which is fine since we already maintain an array of all files,
and an array of filtered files. For directories, since they don't get filtered
normally, there is no separate array. So, we check the hidden-ness as the
directory is read in. Now that I think about it, if we're going to have to read
in the directory again to change the hidden files pref, we might as well filter
hidden files there as well as hidden directories. A bit of a non-issue though,
since there's no UI for telling us to show hidden files.
>301 for (chr = aFilterString; *chr; ++chr) {
>306 // ; will be followed by a space, and then the next filter
>307 chr += 2;
>
>If someone passes in a string like "abc;", this won't deal all too cleanly with
>it. Can this ever happen?
No. The list of filters is in the properties file, the user can't enter a string.
>Also, this should be |++chr;|, I think:
>
>"abc; def"
>
>When you find the ';', you copy "abc", then make |chr| point at the 'd'. Then
>the |for| loop increments |chr|, which now points at 'e'. Not too evil, I >guess,
>except you'll break if your input is ever something like "abc; ; def", since
>|aPos| (which should really just be named |pos|) now points at the second ';'
>and will never be fixed since |chr| never was able to check it.
Again, we don't need to worry about handling arbitrary strings.
>328 if (mOutliner) {
>329 mOutliner->RowCountChanged(dirCount, newFileRows - oldFileRows);
>330
>331 PRInt32 commonRange = PR_MIN(newFileRows, oldFileRows);
>332 if (commonRange)
>328 if (mOutliner) {
>329 mOutliner->RowCountChanged(dirCount, newFileRows - oldFileRows);
>330
>331 PRInt32 commonRange = PR_MIN(newFileRows, oldFileRows);
>332 if (commonRange)
>333 mOutliner->InvalidateRange(dirCount, dirCount + commonRange);
>334 }
>
>I thought you said this did weird things when you tried that in |SetDirectory|?
>Why does this work here, but not for |SetDirectory|?
I'll check, this might cause odd scrollbar problems too.
>347 if (0 <= currentIndex) {
>
>|if (currentIndex >= 0)|, _please!_
Why?
>351 nsCOMPtr<nsISupports> elem =
>dont_AddRef(mDirList->ElementAt(currentIndex));
>352 CallQueryInterface(elem, aFile);
>
>I think you could do:
>
>mDirList->QueryElementAt(currentIndex, NS_GET_IID(nsIFile), aFile));
>
>and spare an AddRef and Release. Same thing a little lower.
Good point. Fixed.
>356 if ((currentIndex - dirCount) < fileCount) {
>
>Could that be simplified to |if (currentIndex < mTotalRows)|?
>
>409 else if ((aRow - dirCount) < fileCount)
>
>Could this be simplified to |else if (aRow < mTotalRows)|?
Both fixed.
>512 if (aRow < (PRInt32) dirCount) {
>
>515 } else if ((aRow - dirCount) < fileCount) {
>
>You should cast fileCount to PRInt32 too, then.
The compiler didn't warn about this line, for whatever reason. I suppose to be
technically correct that we are throwing away unsignedness, it should be:
} else if ((aRow - (PRInt32) dirCount) < (PRInt32) fileCount) {
>518 } else {
>519 // invalid row
>520 *aCellText = ToNewUnicode(NS_LITERAL_STRING(""));
>521 return NS_OK;
>522 }
>
>Will this ever happen? If so, shouldn't you then also check that we're not only
>displaying directories before returning the text on a file? In other words,
>shouldn't it be:
>
>515 } else if (aRow < mTotalRows) {
>
>See also lines 356 and 409.
Yes, this can happen, or at least it used to. The outliner would ask for a row
one off the end. I fixed the comparison.
>541 } else
>542 *aCellText = ToNewUnicode(NS_LITERAL_STRING(""));
>
>Will this ever happen?
No, just trying to be complete. Removed it and optimized the last else.
>622 for (PRUint32 i = 0; i < count; ++i) {
>623 nsCOMPtr<nsIFile> aFile = do_QueryElementAt(mFileList, i);
>
>just call this |file|, it's not an argument, so no need for the |a|. Also, I
>believe it's faster to declare the nsCOMPtr outside the loop and reuse it.
Fixed.
>632 for (PRInt32 i = 0; i < filterCount; ++i) {
>
>You're hiding the outer |i| here (I'm sure the compiler warned about this).
>Nothing wrong with that per se, except for those crappy compilers that don't do
>scoping in |for| loops well, so you'll get a compile error for "redeclaring
>variable" or some such.
Actually, it didn't warn. Changed the variable name just to be safe.
>737 nsCOMPtr<nsISupports> item = aArray->ElementAt(i);
>738 CallQueryInterface(item, &(array[i]));
>
>I'm still wondering if you could do:
>
>aArray->QueryElementAt(i, NS_GET_IID(nsIFile), &(array[i]));
>
>and spare an AddRef and Release. See also up somewhere near line 350.
Yep, fixed. Thanks for the thorough review -- I'm checking in all of the
changes I said I was making here.
Comment 15•23 years ago
|
||
fwiw the C++ guidelines on mozilla.org say not to redeclare variable names.
Comment 16•23 years ago
|
||
Comment on attachment 56060 [details] [diff] [review]
patch to turn on new filepicker
sr=ben@netscape.com
Attachment #56060 -
Flags: superreview+
Comment 17•23 years ago
|
||
This got checked in,
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fxpfe%2Fcomponents%2Ffilepicker&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=day&mindate=&maxdate=&cvsroot=%2Fcvsroot,
and with that change, I can't use the filepicker no more. I backed out locally,
and everything was fine again.
Symptoms:
No display in the filepicker, no way to close the filepicker, or mozilla. Even
the windowmanager can't kill it, I had to kill the mozilla process.
solaris, gcc, fvwm2
Should we backout?
Comment 18•23 years ago
|
||
Axel, I haven't had an opportunity to try this
myself yet, but have you tried removing your
component.reg file?
Comment 19•23 years ago
|
||
deleting component.reg helped, thanx
Assignee | ||
Comment 20•23 years ago
|
||
Ok, the new filepicker code is turned on. I think this is about as fast as it's
going to get...
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 21•23 years ago
|
||
it took 1.47sec to list the contents of /dev [filter set to "all"] on my
pIII-500mhz; tested on rh7.2, 2001.11.27.12-comm bits. in comparison, it took
6.57sec on the same machine, same profile with a build from 2001.10.31.08-comm.
marking vrfy'd.
Status: RESOLVED → VERIFIED
Updated•20 years ago
|
Product: Core → Mozilla Application Suite
You need to log in
before you can comment on or make changes to this bug.
Description
•