Closed Bug 100828 Opened 23 years ago Closed 23 years ago

nsLocalFileMac has serious problems

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Platform:
x86
Windows 2000
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.8

People

(Reporter: dougt, Assigned: ccarlen)

References

Details

Attachments

(1 file, 4 obsolete files)

updated patch
182.39 KB, patch
Details | Diff | Splinter Review
There are serious nsCOMPtr usage problems which cause preformance problem. This file needs a nice overhaul.
Blocks: 27079
It needs an overhaul for other reasons as well - too many redundant instance vars for one. I'm hoping to get to that in 0.9.6. What are the nsCOMPtr uses which cause perf problems?
great. over to you. maybe a dup. Check for cases where we assign our base class into a comptr for the fun of it. Also, for excessive QI's.
Assignee: dougt → ccarlen
Reworking it to address these problems: (1) It doesn't observe the followLinks attribute - perf problem because we always attempt to check for and resolve aliases on every ResolveAndStat(). It will have to default to TRUE for debug builds but could help on release builds. (2) nsILocalFileMac does not have an .idl file and the iface does not inherit from nsILocalFile. This is a huge pain. Whenever making an nsILocalFileMac with an NS_NewLocalFileWithFSSpec, you have to QI the stupid thing before using it as an nsIFile. (3) The directory iterator is really inefficient - see this mess: http://lxr.mozilla.org/mozilla/source/xpcom/io/nsLocalFileMac.cpp#924 (4) Making better use of the cached cat info This patch will also fix bug 28966
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.6
Attached file prelim patch (obsolete) —
Now it has an actual .idl file and descends from nsILocalFile which makes using it much better - notice lack of annoying QI's in PPEmbed. Two things I'm not sure about: (1) fileCreator and type are separate attributes which seems right to me. Although, most consumers get both at the same time so it adds a line of code here and there. (2) There is a stack object which sets and restores the state of the followLinks attribute. This saves a few lines of code an restores the state on early function exit. It's defined locally in nsLocalFileMac.cpp. Should it be in the header of nsILocalFile?
I took some before patch/after patch performance measurements. I measured the time it took for xptiInterfaceInfoManager::BuildFileList() to enumerate the components dir which happens twice each time at startup. I did this with a debug build but I resolved all the aliases using drop stuff/stuffit expander to simulate a release build. In both cases, the 1st scan of the dir takes more time than the 2nd (probably because of OS level file mgr caching) 1st scan 2nd scan before patch: 101 ticks 24 ticks after patch: 3 ticks 2 ticks That means this patch saves 120 ticks (2 seconds!) on directory enumeration on every freakin' launch. And this was only the enumeration done by xptiInterfaceInfoManager::BuildFileList() Caveat: in a debug build, there are more items in the components dir because xSYM files are present but still, the gain is pretty big. I'm posting a new patch to nsLocalFileMac as some bugs were discovered in the prelim patch.
Attached file updated patch to nsLocalFileMac (obsolete) —
Conrad: there are problems with the cached CInfoPBRec. Basically, it's impossible for us to maintain the dirty state of this cache, because calls outside of nsLocalFileMac can invalidate it. For example, we have code that uses the OpenNSPRFileDesc (or whatever) to open a file for writing outside of this code. Writing to the file will invalidte the cached CInfoPBRec, but there's no way we can know that it's changed (other than to check the mod date, which you'd do with PBGetCatInfo anyway). Note also that there are some new file APIs using FSRefs which allow for getting file info in batches. This would be good for the enumerators.
Also xpti enumration happens only for debug builds (right jband ?) There are other enumerations happening though on release builds. Will this patch save on any enumeration ?
dp: yes, the xpt file enumeration happens only in debug builds, if the xpti.dat is unreadable, or we are doing an explicit xpcom autoregistration. If it is a useful benchmark, then great. But don't count on speeding up this particular directory enumeration to matter for release builds.
> Conrad: there are problems with the cached CInfoPBRec. * I could probably ditch that optimization. I tested the performance again, forcing UpdateCachedCatInfo() to always update and it did not lessen the perf gain of enumeration. The current enumeration code is slow for several other reasons. * Still, we could keep it and be smarter about when to invalidate it. The methods IsFile() and IsDirectory() are called a lot. Even if something opens the file and changes it, whether it's a file or directory is not going to change. So, GetFileSize() and GetLastModificationDate() could always force an update of the cached info, whereas IsFile() and IsDirectory() would not. > Note also that there are some new file APIs using FSRefs Those will be used when the nsLocalFile impl is factored into Classic, HFSPlus, and OSX versions. There's already too much piecemeal OS X code in here now. > There are other enumerations happening though on release builds. Will this > patch save on any enumeration ? Yes. I measured xptiInterfaceInfoManager::BuildFileList() because it does little else other than the enumeration so measuring it is close to measuring only the enumeration. But still, any enumeration done at startup will benefit.
Attached file updated patch to mozilla/xpcom/io (obsolete) —
The update contains improvements and fixes after running it through http://jimbob/trigger3.html. The xpinstall test script is good because it makes extensive use of nsILocalFile. Another change: UpdateCachedCatInfo takes a boolean parameter with which an update can be forced. This is used in most places - particularly when getting the date, size, or existence. The original code was caching this info far too long. See mHaveFileInfo. The only thing which reset this member was MakeDirty() which reset everything which isn't nescesary. Even with updating the cached cat info more frequently, dir enumeration is still tremendously faster.
Conrad: can you obsolete the old patches please?
Attachment #53708 - Attachment is obsolete: true
Attachment #53708 - Attachment is patch: false
Simon, the prelim patch contains tree-wide changes for nsILocalFileMac API changes. At this point, those will not change. The changes in attachment 54483 [details] supercede any changes in the first patch to mozilla/xpcom/io. It's not quite ready for review yet, though. I'm currently getting rid of the internal use of eInitWithPath/eInitWithFSSpec. Assuming that works out, there'll be another patch.
*** Bug 104248 has been marked as a duplicate of this bug. ***
I have a new patch coming up which does a few things: (1) Gets rid of eInitializedWithPath, eInitializedWithFSSpec distinction. When initialized with a full patch, the existant portion of the path is resolved into a spec and the rest, which doesn't exist, is kept in mAppendedPath. When initializing with a full path, at least the volume must exist or else it's an error. (2) The object used to contain 3 FSSpecs and 3 path member vars - not very concise. The object now contains 2 specs and only one path. The reason for 2 specs is so that, is the leaf is an alias, we can toggle back and forth between resolving the alias or not. (3) When Append() is called, the current leaf is resolved if it is an alias. Before this wasn't done and alises were only resolved at the leaf when the patch was finally built up. If one of the appended nodes was an alias, it would fail. The problem, to a user, was that you could not make an alias of your cache directory (or much of anything else) Now you can.
Attached patch new tree-wide patch (obsolete) — — Splinter Review
Attachment #53312 - Attachment is obsolete: true
Attachment #53312 - Attachment is patch: false
Attachment #54483 - Attachment is obsolete: true
Attachment #54483 - Attachment is patch: false
This one is ready for review. Another thing I would like to do, since so much of this is being rewritten, is to get rid of the tabs that have always been present thoughout this file. As you can see, it makes diffs ugly if the new code uses spaces. I could do one of two things - reviewers take your choice: 1. review as-is and, before checking in, I'll detab it with BBEdit. 2. detab it now and post a new diff made with -w
Do this one: 1. review as-is and, before checking in, I'll detab it with BBEdit.
Alright, then review away. I'll detab it before checking in.
*** Bug 28966 has been marked as a duplicate of this bug. ***
Comments: + attribute OSType fileType; + attribute OSType creator; Use 'fileCreator' for consistency? + char mAutoBuffer[512]; + char *mAllocatedBuffer; + char *mNewString; We really need a class that does this stack/buffer thing transparently. It's used in so many places. + return nsCRT::strtok(mAllocatedBuffer ? mAllocatedBuffer : mAutoBuffer, ":", &mNewString); Why not store a pointer than can point to either the allocated buffer, or the stack buffer, and avoid this if/else test all the time? You'd just have to set it up in the ctor. The rest looks OK, but this patch needs some thorough testing, since it's so core. I'd even suggest testing an installer, to make sure that installation didn't break. sr=sfraser
> Use 'fileCreator' for consistency? OK > Why not store a pointer than can point to either the allocated buffer OK - It was to save an instance var in favor of the low expense of the if/else. Now, I think what you suggest is better. As far as testing with the installer, I did test successfully with http://jimbob/trigger3.html which hits every obscure case. I still need an r= on this. Patrick or Doug? Since the changes mentioned above are slight, please review the current patch.
Attachment #54920 - Flags: superreview+
CC'ing rjc for review. This has had sr= for a while. Now if I can get an r=.
o attribute OSType creator; I second the "fileCreator" instead of "creator" change. I assume that the method name (not just the variable name) was also changed ... GetFileCreator() instead of GetCreator() o *outIsWritable = !(mCachedCatInfo.hFileInfo.ioFlAttrib & 0x01); Nit: instead of 0x01, suggest using the correct mask from <Files.h> which looks to be kioFlAttribLockedMask Also, I've always been quite annoyed by our alias resolution handling on Mac. SO... I'd like to see the following test done. :) 1) mount a remote Appletalk volume (call it _VOL_ for short) 2) make an alias to that remote volume (call it _AL_ for short) 3) unmount the remote volume 4) place the alias in some directory (call it _DIR_ for short) on the local hard disk 5) with Mozilla, type in the appropritate file:/// URL to view the contents of directory _DIR_ If Mozilla seems to go to sleep for up to two minutes (as it incorrectly tries to mount _VOL_) or anything action other than merely listing the alias _AL_ itself, then something is still wrong with alias resolution. With the above changes, and IFF Mozilla passes the above test correctly, r=rjc
I'll do the first two points. As far as this: 1) mount a remote Appletalk volume (call it _VOL_ for short) 2) make an alias to that remote volume (call it _AL_ for short) 3) unmount the remote volume 4) place the alias in some directory (call it _DIR_ for short) on the local hard disk 5) with Mozilla, type in the appropritate file:/// URL to view the contents of directory _DIR_ It currently defaults to resolving aliases. It has to for most consumers to work. One thing this patch does is to make nsLocalFile obey the followLinks attribute. With this patch, you could at least set the follow links attribute to FALSE on each entry you got back in the enumeration and it would actually do something. I think what would be more convenient and offer better control would be to add a param to GetEntries() specifying whether or not to resolve aliases on each entry that's returned. You may want to resolve the alias to the dir being enumerated but not resolve the children. That's another bug though.
Robert, I will try putting a call to SetFollowLinks(PR_FALSE) in the directory listing generator as soon as it gets back an entry and make sure it does the right thing.
Conrad: SetFollowLinks(PR_FALSE) is already in the RDF file system datasource which is what actually ends up handling file:/// URL display. :) So the test should be a good one to do.
At this point, right: http://lxr.mozilla.org/mozilla/source/rdf/datasource/src/nsFileSystemDataSource.cpp#1215 That's not quite going to do it. It should be done also on line 1233. Say in this case, aDirLocal was an alias to a directory. You may want to resolve that. But, what you don't want resolved are its entries. The additional call on line 1233 will do that.
Agreed on line # 1233 due to the IsHidden() call; however I don't know that I agree that aliases to directories should ever be resolved. Sticky issue... if I wanted to delete the alias to a directory (assuming that we actually had that functionality, that is), I wouldn't want the real directory to be blown away. Anyway, different issue for a different time/bug. :)
Mass move to 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Better to get something like this into the beginning of 0.9.8 than end of 0.9.7
Target Milestone: mozilla0.9.7 → mozilla0.9.8
Attached patch updated patch — — Splinter Review
Update to the last patch - no significant changes, just updated for changes in the tree. nsLocalFileMac.cpp was de-tabbed as this is the final which will be checked in.
Attachment #54920 - Attachment is obsolete: true
Checked in. nsLocalFileMac was de-tabbed so pretty much everything is different. After the tree closed, I remembered that the old nsILocalFileMac.h needs to be removed. I'll leave this open until I do that later today.
The handmade nsILocalFileMac.h is removed so marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: