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)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.7

People

(Reporter: jg, Assigned: bryner)

References

Details

(Keywords: perf, Whiteboard: [nav+perf])

Attachments

(1 file)

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.
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.
Blocks: qt
-> bryner
Assignee: waterson → bryner
Component: XP Miscellany → XP Apps: GUI Features
QA Contact: brendan → sairuh
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
Whiteboard: [nav+perf]
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
Blocks: 91351
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
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
This is a Linux bug. It is not a win32 bug. No morphing allowed.
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 ;-)
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
Blocks: 74634
Target Milestone: mozilla0.9.6 → mozilla0.9.7
The C++ rewrite for the outliner view is now checked in (turned off). I'm attaching a patch to turn it on.
Attached patch patch to turn on new filepicker (deleted) — Splinter Review
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.
>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.
fwiw the C++ guidelines on mozilla.org say not to redeclare variable names.
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?
Axel, I haven't had an opportunity to try this myself yet, but have you tried removing your component.reg file?
deleting component.reg helped, thanx
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
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
Component: XP Apps: GUI Features → UI Design
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: