82854 - FilePicker: Loading a directory with large number of files is very slow

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[James Green]

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.

Comment 1

[James Green]

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.

Comment 2

[Chris Waterson]

-> bryner

Comment 3

[Brian Ryner (not reading)]

I think this is about as fast as it's going to get without some work on
nsLocalFileUnix.

Comment 4

[James Green]

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.

Comment 5

[Brian Ryner (not reading)]

moving these out to 0.9.5 since it doesn't look like I will have time for 0.9.4.

Comment 6

[Vicente Salvador]

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

[John Morrison]

This is a Linux bug. It is not a win32 bug. No morphing allowed.

Comment 8

[jag (Peter Annema)]

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

[Peter Trudelle]

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.

Comment 10

[Brian Ryner (not reading)]

The C++ rewrite for the outliner view is now checked in (turned off).  I'm
attaching a patch to turn it on.

Comment 11

[Brian Ryner (not reading)]

Attachment: patch to turn on new filepicker

Comment 12

[jag (Peter Annema)]

http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=day&who=bryner%25netscape.com

Comment 13

[jag (Peter Annema)]

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.

Comment 14

[Brian Ryner (not reading)]

>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

[timeless]

fwiw the C++ guidelines on mozilla.org say not to redeclare variable names.

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 56060 [details] [diff] [review]
patch to turn on new filepicker

sr=[e-mail]

Comment 17

[Axel Hecht]

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

[Adam D. Moss]

Axel, I haven't had an opportunity to try this
myself yet, but have you tried removing your
component.reg file?

Comment 19

[Axel Hecht]

deleting component.reg helped, thanx

Comment 20

[Brian Ryner (not reading)]

Ok, the new filepicker code is turned on.  I think this is about as fast as it's
going to get...

Comment 21

[sairuh (rarely reading bugmail)]

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.