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

VERIFIED FIXED in mozilla0.9.7

Status

SeaMonkey
UI Design
VERIFIED FIXED
17 years ago
10 years ago

People

(Reporter: James Green, Assigned: Brian Ryner (not reading))

Tracking

({perf})

Trunk
mozilla0.9.7
x86
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nav+perf])

Attachments

(1 attachment)

6.35 KB, patch
Ben Goodger (use ben at mozilla dot org for email)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

17 years ago
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

17 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.
Keywords: mozilla0.9.2, mozilla1.0, perf

Updated

17 years ago
Blocks: 79120

Comment 2

17 years ago
-> bryner
Assignee: waterson → bryner
Component: XP Miscellany → XP Apps: GUI Features
QA Contact: brendan → sairuh
(Assignee)

Comment 3

17 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

17 years ago
Whiteboard: [nav+perf]
(Reporter)

Comment 4

17 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.
Target Milestone: mozilla0.9.3 → mozilla0.9.4

Updated

17 years ago
Blocks: 91351
(Assignee)

Comment 5

17 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

17 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

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

Comment 8

17 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

17 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

Updated

17 years ago
Blocks: 74634

Updated

17 years ago
Target Milestone: mozilla0.9.6 → mozilla0.9.7
(Assignee)

Comment 10

17 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

17 years ago
Created attachment 56060 [details] [diff] [review]
patch to turn on new filepicker

Comment 13

17 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

17 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

17 years ago
fwiw the C++ guidelines on mozilla.org say not to redeclare variable names.
Comment on attachment 56060 [details] [diff] [review]
patch to turn on new filepicker

sr=ben@netscape.com
Attachment #56060 - Flags: superreview+

Comment 17

17 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

17 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

17 years ago
deleting component.reg helped, thanx
(Assignee)

Comment 20

17 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
Last Resolved: 17 years ago
Resolution: --- → FIXED
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
Product: Core → Mozilla Application Suite

Updated

10 years ago
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.