Closed Bug 164396 Opened 18 years ago Closed 17 years ago

reduce conversions from path <-> FSRef and fix nsLocalFileOSX bugs

Categories

(Core :: XPCOM, defect, P1, critical)

PowerPC
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla1.4beta

People

(Reporter: Brade, Assigned: ccarlen)

References

()

Details

(Keywords: perf)

Attachments

(1 file, 3 obsolete files)

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?
Keywords: perf
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).
Assignee: brade → ccarlen
Status: NEW → ASSIGNED
Summary: reduce conversions from path -> FSRef/FSSpec → reduce conversions from path <-> FSRef
Target Milestone: --- → Chimera0.6
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.
Component: General → XPCOM
Product: Chimera → Browser
Target Milestone: Chimera0.6 → mozilla1.3alpha
Version: unspecified → other
Mass move of 1.3a bugs -> 1.4a.
Target Milestone: mozilla1.3alpha → mozilla1.4alpha
Using FSRef to specify files has bigger problems than performance. See bug
192124. Upping severity and priority.
Severity: normal → critical
Priority: -- → P1
Attached patch patch (obsolete) — Splinter Review
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 ;-)
Blocks: 164189
Flags: blocking1.4a?
Summary: reduce conversions from path <-> FSRef → reduce conversions from path <-> FSRef and fix nsLocalFileOSX bugs
Attached patch patch.2 (obsolete) — Splinter Review
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.
Attachment #117222 - Attachment is obsolete: true
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.
Attachment #117557 - Flags: superreview+
Attached patch with simon's comments addressed (obsolete) — Splinter Review
> 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.
Attachment #117557 - Attachment is obsolete: true
Attachment #117653 - Flags: review?(sdagley)
Attachment #117653 - Flags: review?(sdagley) → review+
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.
Attachment #117653 - Attachment is obsolete: true
Flags: blocking1.4a? → blocking1.4a-
-> 1.4b
Target Milestone: mozilla1.4alpha → mozilla1.4beta
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
Blocks: 196119
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.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.