We should reduce the number of times we call FSPathMakeFSSpec and FSPathMakeFSRef. Maybe we can store the FSRef in the nsLocalFile* implementations? Maybe we can lazily validate the url/file when file names are appended?

Should this be a generic Mozilla bug?

hmm - there's no such call as FSPathMakeFSSpec, FSPathMakeFSRef is really called FSPathMakeRef, and it's only used once in nsLocalFile::GlobalInit(). Kathy, since FSPathMakeFSSpec doesn't exist, can you explain what you're talking about WRT FSSpecs? Here's what I think should happen though: Instead of representing the file object as an FSRef + appended nodes, It should just be a CFURL. Reason is, nsLocalFile::GetNativePath and nsLocalFile::InitWithNativePath are used heavily. When initializing, the given path needs to be decomposed to an FSRef (though not using FSPathMakeRef). And, on getting the path, FSRefMakePath is used on the internal FSRef. If the internal representation was a path (CFURL) those two common operations would be much less expensive.

reassign to ccarlen for conversion of nsLocalFileOSX to use CFURL I mistyped in my original comments. FSRefMakePath is called by both nsLocalFile::GetPath and nsLocalFile::GetNativePath. nsFileIO seems to be the biggest user of calling these (during page load).

To see what the expense of this was, I put in some code to measure time spent in GetPath and GetNativePath using PR_IntervalNow() at the top and bottom of the functions along with a counter as to how many calls were made. Printing the data at xpcom shutdown time gave: Total time for 708 calls to GetPath/GetNativePath = 239 millisecs. This was after starting Chimera and visiting mozilla.org, netscape.com, and cnn.com. I was surprised at the low number of calls (708) compared to what Kathy found.

Not just Chimera anymore.

Mass move of 1.3a bugs -> 1.4a.

Using FSRef to specify files has bigger problems than performance. See bug 192124. Upping severity and priority.

Total reworking of nsLocalFileOSX.cpp. It no longer represents the file internally as an FSRef - which is the root of quite a few problems. Instead, uses a CFURL. I tested performance by building with --enable-chrome-format=flat, which *really* hammers the file impl at startup, and measured the Ts results. They're good :-) old impl: 5377 new impl: 4493

Nice. Do we have a test suite that was can run this against?

We do need a thorough nsIFile test suite. 1. There's nsIFileTest, but it only exercises a small subset of the API. 2. Pete Collins wrote some tests in JS which might be good - doubt they're current with the API. See bug 94323. 3. The xpinstall tests are good at finding file bugs - I'll run that suite in addition to other testing. Hey, this just might give us enough improvement on Ts to afford the hit of the splash screen ;-)

This adds a boolean param to GetFSRefInternal which controls whether a new FSRef has to be made from the CFURL. Since the point of this patch is to get rid of problems caused by stale FSRefs, using the cached FSRef is done very rarely. Also needed to remove a change to xpti loader which was made for the sake of the current nsLocalFileOSX code. If an alias is encountered and followLinks is off, the returned size of the file will be 0. This is because the contents of a Finder alias are all in the resource fork.

Comment on attachment 117557 [details] [diff] [review] patch.2 sr=sfraser on addressing the following: > NS_METHOD nsLocalFile::nsLocalFileConstructor(nsISupports* outer, const nsIID& aIID, void* *aInstancePtr) > { > NS_ENSURE_ARG_POINTER(aInstancePtr); > NS_ENSURE_NO_AGGREGATION(outer); > > nsLocalFile* inst = new nsLocalFile(); > if (inst == NULL) > return NS_ERROR_OUT_OF_MEMORY; > > nsresult rv = inst->QueryInterface(aIID, aInstancePtr); > if (NS_FAILED(rv)) > { > delete inst; > return rv; > } > return NS_OK; > } Should this have an AddRef/Release around the QI, to ensure that the object is kept alive? > /* void create (in unsigned long type, in unsigned long permissions); */ > NS_IMETHODIMP nsLocalFile::Create(PRUint32 type, PRUint32 permissions) > { > if (type != NORMAL_FILE_TYPE && type != DIRECTORY_TYPE) > return NS_ERROR_FILE_UNKNOWN_TYPE; > > nsStringArray nonExtantNodes; > CFURLRef pathURLRef = mBaseRef; > FSRef pathFSRef; > CFStringRef leafStrRef = nsnull; > StBuffer<UniChar> buffer; A quick comment here saying what the code is doing would be great. > PRBool success; > > while ((success = ::CFURLGetFSRef(pathURLRef, &pathFSRef)) == PR_FALSE) { Strictly speaking, CFURLGetFSRef returns a Boolean, not a PRBool. > leafStrRef = ::CFURLCopyLastPathComponent(pathURLRef); > if (!leafStrRef) > break; ... > /* attribute AString leafName; */ > NS_IMETHODIMP nsLocalFile::GetLeafName(nsAString& aLeafName) > { > nsCAutoString nativeString; > nsresult rv = GetNativeLeafName(nativeString); > if (NS_FAILED(rv)) > return rv; > aLeafName.Assign(NS_ConvertUTF8toUCS2(nativeString)); > return NS_OK; > } Wouldn't it be slightly more efficient to call CFURLCopyLastPathComponent directly, then make a CFStringReftoUCS2? That would avoid the CFStringReftoUTF8 followed by the NS_ConvertUTF8toUCS2(). > /* readonly attribute AString path; */ > NS_IMETHODIMP nsLocalFile::GetPath(nsAString& aPath) > { > nsCAutoString nativeString; > nsresult rv = GetNativePath(nativeString); > if (NS_FAILED(rv)) > return rv; > aPath.Assign(NS_ConvertUTF8toUCS2(nativeString)); > return NS_OK; > } Again, maybe avoid the two copies by calling CFURLCopyFileSystemPath directly. > /* readonly attribute nsIFile parent; */ > NS_IMETHODIMP nsLocalFile::GetParent(nsIFile * *aParent) > { > nsresult rv = NS_ERROR_FAILURE; > NS_ENSURE_ARG_POINTER(aParent); > *aParent = nsnull; > > nsLocalFile *newFile = nsnull; > > // If it can be determined without error that a file does not > // have a parent, return nsnull for the parent and NS_OK as the result. > // See bug 133617. > CFURLRef parentURLRef = ::CFURLCreateCopyDeletingLastPathComponent(kCFAllocatorDefault, mBaseRef); > if (parentURLRef) { > newFile = new nsLocalFile; > if (newFile) { > rv = newFile->InitWithCFURL(parentURLRef); > if (NS_SUCCEEDED(rv)) { > NS_ADDREF(*aParent = newFile); > rv = NS_OK; I think parentURLRef is leaked here. > } > } > ::CFRelease(parentURLRef); > } > return rv; > } > /* [noscript] void appendRelativeNativePath (in ACString relativeFilePath); */ > NS_IMETHODIMP nsLocalFile::AppendRelativeNativePath(const nsACString& relativeFilePath) > { > if (relativeFilePath.IsEmpty()) > return NS_OK; > // No leading '/' > if (relativeFilePath.First() == '/') > return NS_ERROR_FILE_UNRECOGNIZED_PATH; > > // Parse the nodes and call Append() for each > nsACString::const_iterator nodeBegin, pathEnd; > relativeFilePath.BeginReading(nodeBegin); > relativeFilePath.EndReading(pathEnd); > nsACString::const_iterator nodeEnd(nodeBegin); > > while (nodeEnd != pathEnd) { > FindCharInReadable(kPathSepChar, nodeEnd, pathEnd); > nsresult rv = AppendNative(Substring(nodeBegin, nodeEnd)); > if (NS_FAILED(rv)) > return rv; > if (nodeEnd != pathEnd) // If there's more left in the string, inc over the '/' nodeEnd is on. > ++nodeEnd; > nodeBegin = nodeEnd; > } > return NS_OK; > } Is there a CFURL way to append more than one component at a time? > /* CFURLRef getCFURL (); */ > NS_IMETHODIMP nsLocalFile::GetCFURL(CFURLRef *_retval) > { > NS_ENSURE_ARG_POINTER(_retval); > *_retval = nsnull; > > FSRef fsRef; > nsresult rv = GetFSRefInternal(fsRef); > if (NS_SUCCEEDED(rv)) { > *_retval = ::CFURLCreateFromFSRef(NULL, &fsRef); > } > else { > nsCAutoString path; > rv = GetPathInternal(path); > if (NS_FAILED(rv)) > return rv; > CFStringRef pathAsCFString; > pathAsCFString = ::CFStringCreateWithCString(nsnull, path.get(), kCFStringEncodingUTF8); > if (!pathAsCFString) > return NS_ERROR_FAILURE; > *_retval = ::CFURLCreateWithFileSystemPath(nsnull, pathAsCFString, kCFURLPOSIXPathStyle, PR_FALSE); > ::CFRelease(pathAsCFString); > } > return *_retval ? NS_OK : NS_ERROR_FAILURE; > } Why can't this just return mBaseRef or mTargetRef? Also, this needs some comments to say whether the caller should CFRelase the return value (since it doesnt' use the OS convention of having 'Copy' in the name). > /* FSRef getFSRef (); */ > NS_IMETHODIMP nsLocalFile::GetFSRef(FSRef *_retval) > { > NS_ENSURE_ARG_POINTER(_retval); > return GetFSRefInternal(*_retval); > } > > /* FSSpec getFSSpec (); */ > NS_IMETHODIMP nsLocalFile::GetFSSpec(FSSpec *_retval) > { > NS_ENSURE_ARG_POINTER(_retval); > > OSErr err; > FSRef fsRef; > nsresult rv = GetFSRefInternal(fsRef); > if (NS_SUCCEEDED(rv)) { > // If the leaf node exists, things are simple. > err = ::FSGetCatalogInfo(&fsRef, kFSCatInfoNone, > nsnull, nsnull, _retval, nsnull); > return MacErrorMapper(err); > } > else if (rv == NS_ERROR_FILE_NOT_FOUND) { > // If the parent of the leaf exists, make an FSSpec from that. > CFURLRef parentURLRef = ::CFURLCreateCopyDeletingLastPathComponent(kCFAllocatorDefault, mBaseRef); > if (!parentURLRef) > return NS_ERROR_FAILURE; > > err = fnfErr; > if (::CFURLGetFSRef(parentURLRef, &fsRef)) { > FSCatalogInfo catalogInfo; > if ((err = ::FSGetCatalogInfo(&fsRef, > kFSCatInfoVolume + kFSCatInfoNodeID + kFSCatInfoTextEncoding, > &catalogInfo, nsnull, nsnull, nsnull)) == noErr) { > nsAutoString leafName; > if (NS_SUCCEEDED(GetLeafName(leafName))) { > Str31 hfsName; > if ((err = ::UnicodeNameGetHFSName(leafName.Length(), > leafName.get(), > catalogInfo.textEncodingHint, > catalogInfo.nodeID == fsRtDirID, > hfsName)) == noErr) > err = ::FSMakeFSSpec(catalogInfo.volume, catalogInfo.nodeID, hfsName, _retval); This will return fnfErr if the file doesn't exist, but we return a good FSSpec in this case. Should we ignore the error in this case? > } > } > } > ::CFRelease(parentURLRef); > rv = MacErrorMapper(err); > } > return rv; > } > nsresult nsLocalFile::SetBaseRef(CFURLRef aCFURLRef) Needs comments about ownership.

> Should this have an AddRef/Release around the QI, to ensure that the object is kept alive? Since this has a standard QI, it's OK as it is. I double-checked with alecf. > A quick comment here saying what the code is doing would be great. Added it. > Strictly speaking, CFURLGetFSRef returns a Boolean, not a PRBool. Changed it to Boolean and true. > Wouldn't it be slightly more efficient to call CFURLCopyLastPathComponent Explained in a comment about the general impl in nsLocalFileOSX.h. It's for consistency, and the conversion is cheap. > I think parentURLRef is leaked here. I don't see it. It's refcnt == 1 after it gets created, == 2 after the call to InitWithCFURL, and is back to 1 after the CFRelease below. > Is there a CFURL way to append more than one component at a time? I don't think so - without being a compound URL. I tried CFURLCreateFromFileSystemRepresentationRelativeToBase() and it behaved strangely. > Why can't this just return mBaseRef or mTargetRef? Also, this needs > some comments to say whether the caller should CFRelase the return value You're right - I totally simplified it to do that. As far as the comments, it has just such comments in the idl. > This will return fnfErr if the file doesn't exist, but we return a good > FSSpec in this case. Should we ignore the error in this case? It is Mac OS FSSpec convention to return fnfErr in that case. it's what I'd expect. > Needs comments about ownership. Added them.

This was checked in last night, but drove Ts on monkey up by around 50% so I backed it out. My own Ts tests before checking in showed the patch improved things by about 20%. Not sure why the difference in results.

Contains additional null checks on mBaseRef. I discovered the need for that on perf testing I've been doing.

-> 1.4b

Here's numbers from mocha, g2/6.4: (entry#, time, minValue, data) 1323 2003:04:02:14:22:52 10239 11611 10239 10559 10246 10457 10401 10465 10381 10555 10268 1324 2003:04:02:14:33:46 10225 11736 10347 10571 10225 10608 10243 10485 10279 10562 10304 1325 2003:04:02:14:44:46 10362 11582 10458 10562 10439 10410 10417 10362 10415 10436 10439 1326 2003:04:02:14:55:47 10257 11694 10393 10408 10359 10462 10335 10559 10257 10484 10281 1327 2003:04:02:15:06:50 10221 11561 10278 10441 10221 10422 10361 10625 10339 10605 10283 1328 2003:04:02:15:17:49 10279 11727 10415 10530 10296 10502 10394 10576 10341 10488 10279 [applied patch 2003-03-23 10:19:17 here] 1329 2003:04:02:16:12:17 7905 9680 7908 8315 8078 8247 7989 8329 8067 8370 7905 1330 2003:04:02:16:22:55 7926 9184 8036 8226 8071 8192 8079 8151 8054 8053 7926 1331 2003:04:02:16:33:49 7986 9106 8089 8272 7999 8163 8090 8241 7986 8221 8046 1332 2003:04:02:16:44:28 7884 9243 8080 8297 8030 8246 8023 8237 8029 8222 7884 1333 2003:04:02:16:55:11 7998 9156 8079 8297 7998 8218 8002 8244 8077 8266 8094

Checked in (again) after Ts testing on other Tinderboxen showed similar improvement to what I had found. Also, since this fixes various bugs caused by using FSRefs, it's needed no matter what the effect on Ts.

Is it possible that this fix also increased Txul by a bit less than 30% ? At least that happens on monkey (and nowhere else). Ts on monkey is up by (only) about 20% this time.