Closed Bug 506812 Opened 10 years ago Closed 9 years ago

get rid of FSRef usage in XRE_GetBinaryPath

Categories

(Core :: XPCOM, defect, P2)

All
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad, Assigned: jaas)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ts])

Attachments

(1 file, 1 obsolete file)

From looking at the startup path, there are a few remaining users where we end up using a FSRef, and calling nsLocalFileOSX::InitWithFSRef.  This in turn ends up creating a CFURL, which goes off the deep end and calls getattrlist() on every directory in the parent chain of the target, as best I can tell.

Here are the consumers during startup:

              XUL`nsLocalFile::InitWithFSRef(FSRef const*)+0x148
              XUL`XRE_GetBinaryPath+0x11c
--
              XUL`nsLocalFile::InitWithFSRef(FSRef const*)+0x148
              XUL`nsDirEnumerator::HasMoreElements(int*)+0x278
--
              XUL`nsLocalFile::InitWithFSRef(FSRef const*)+0x148
              XUL`nsXREDirProvider::GetUserDataDirectoryHome(nsILocalFile**, int)+0xc1
              XUL`nsXREDirProvider::GetUserDataDirectory(nsILocalFile**, int)+0x29


The biggest consumer by far is the DirEnumerator one (around 544 out of 578 stacks with InitWithFSRef have DirEnumerator::HasMoreElements).  Each InitWithFSRef is taking around 750-1ms, with a fully warm cache.
Flags: blocking1.9.2?
The XRE_GetBinaryPath one could probably be switched to use CFURLGetFileSystemRepresentation on the CFURL: http://developer.apple.com/documentation/CoreFoundation/Reference/CFURLRef/Reference/reference.html#//apple_ref/c/func/CFURLGetFileSystemRepresentation
and then just Init the localfile with that.

The two GetUserDataDirectory[Home] functions could use NSSearchPathForDirectoriesInDomains instead of FSFindFolder:
http://developer.apple.com/documentation/Cocoa/Conceptual/LowLevelFileMgmt/Tasks/LocatingDirectories.html

I dunno about the nsDirEnumerator one, perhaps josh does.
I don't know if CFURLs are much better, but it's worth a shot.
The only other option AFAICT, would be to move that code into a separate ObjC file and use the Cocoa APIs to get this info, like:
[[NSBundle mainBundle] executablePath]] // returns an NSString*
CFURLs are much better for performance than FSRefs. CFURLs store their target as a URL string, FSRefs store more or less as a disk node reference that has to be resolved via system call just to get a path string.
Hardware: x86 → All
We can just delete the FSRef usage in XRE_GetBinaryPath. Initializing local file objects with a CFURL is actually the fastest way to init since CFURL happens to be our primary backing. We don't even have to create the CFURL object, we just retain it.
Attachment #391652 - Flags: review?(vladimir)
Attachment #391652 - Flags: review?(vladimir) → review+
XRE_GetBinaryPath fix pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/4318f781ab87
Attached patch nsDirEnumerator fix, v1.0 (obsolete) — Splinter Review
I've been meaning to do this for a while but it was lower on the list than some other nsIFile impl cleanup. Seems to work well.
Attachment #391657 - Flags: review?(vladimir)
Assignee: nobody → joshmoz
Comment on attachment 391657 [details] [diff] [review]
nsDirEnumerator fix, v1.0

Also looks good to me..
Attachment #391657 - Flags: review?(vladimir) → review+
I find that hard to believe, since the patch makes us do less work. (And the old codepath basically calls the same code as the new codepath, just with more code around it.)
What Ted said. It is *really* unlikely.
pushed nsDirEnumerator fix to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/ad9a4a3a5409
Making this bug more specific and resolving. We can file new bugs for any other FSRef usage we want to remove.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Summary: Get rid of remaining MacOS X FSRef uses → Get rid of FSRef usage in XRE_GetBinaryPath and nsDirEnumerator
Should file a new bug on nsXREDirProvider::GetUserDataDirectory[Home]
I backed out both changesets here. The first to investigate the Ts change, the second due to talos crashes.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backing out "XRE_GetBinaryPath fix, v1.0" looks to have fix the Ts regression. Amazing and scary.
Joel, another Mac-specific startup issue for you to check out.
Cc'ing the right joelr :)
given this is up to 3 seconds of Mac startup time (per comment #1), this is a P1 bug.

since the Ts regression is Tiger only, and Snow Leopard will already be out by the time these bits ship, do we care?
Priority: -- → P1
Likely not -- if joelr finds that this doesn't have any Ts regressions on the Leopards (and hopefully is a Ts win?), we should take this.
Why not push the changes and see if they cause a regression on Leopard?
I pushed the XRE_GetBinaryPath fix again.

http://hg.mozilla.org/mozilla-central/rev/845b625b5eb5
Backed out due to talos orange. IIRC this didn't happen last time.

http://hg.mozilla.org/mozilla-central/rev/c1e5dcec20dd
This is weird:
Running test ts: 
		Started Wed, 09 Sep 2009 05:47:26
NOISE: 
NOISE: __FAILbrowser crash (code 256)__FAIL
Failed ts: 
		Stopped Wed, 09 Sep 2009 05:57:27
FAIL: Busted: ts
FAIL: failed to initialize browser
Completed test ts: 
		Stopped Wed, 09 Sep 2009 05:57:27

Looks like it took 10 minutes to run (which seems abnormally long), and then returned "1" from main. (the code 256 is the return value of Python's os.system, which makes a 16-bit value out of (return value, signal).)
Assignee: joshmoz → joelr
I installed Talos locally on Snow Leopard and saw neither crashes nor speed gains after applying the patches. Everything ran normally but I still see ~850ms startup from Talos. It must be my 256Gb SSD again. 

What's my next step?
Flags: blocking1.9.2? → blocking1.9.2-
What's next on this? My Talos does not crash and performance is no worse.
I'd say let's try landing it again; even if there's no performance change, the code gets simpler.
I can't land anything, though, and Josh is busy. Vlad, can you land this?
Assignee: joelr → joshmoz
Here we go again. Pushed "XRE_GetBinaryPath fix, v1.0" to mozilla-central.

http://hg.mozilla.org/mozilla-central/rev/65aff8912e7c
Priority: P1 → P2
Backed out again.

http://hg.mozilla.org/mozilla-central/rev/744a11942d04

Talos couldn't even launch the browser. This patch clearly hits a sore point in our local file code and we need to figure out what that is before we try to land it again.

The only thing I can think of is that both InitWithFSRef and InitWithCFURL use SetBaseURL and SetBaseURL caches the URL's path in utf-8 form. It doesn't attempt to validate the path in the way that InitWithPath does, and perhaps the paths from the original URL object in question and the path from the FSRef-derived URL object are different, the former being invalid for our utf-8 cache.
I tried to land "nsDirEnumerator fix, v1.0" again.

http://hg.mozilla.org/mozilla-central/rev/76c570389975

I had to back it out.

http://hg.mozilla.org/mozilla-central/rev/08c4c8853685
http://hg.mozilla.org/mozilla-central/rev/61620a8558c4

I had to back out due to the following unit test failure:

Makefile:327: test_bug419132.html disabled because it takes 60 seconds
Makefile:327: bug419132.html disabled because it takes 60 seconds
Running XMLHttpRequest tests...
NEXT ERROR TEST-UNEXPECTED-FAIL | Couldn't create nsIXMLHttpRequest instance!
Finished running XMLHttpRequest tests.
make[3]: *** [check] Error 1
make[4]: Nothing to be done for `check'.
make[2]: *** [check] Error 2
make[1]: *** [check] Error 2
make: *** [check] Error 2
program finished with exit code 2
I moved the dir enumerator stuff to its own bug (bug 528447) so we don't have two things going on here any more.
Summary: Get rid of FSRef usage in XRE_GetBinaryPath and nsDirEnumerator → get rid of FSRef usage in XRE_GetBinaryPath
I can reproduce the "XRE_GetBinaryPath fix, v1.0" talos crash locally, the crash only happens on 10.5, not 10.6.
Blocks: 447581
Attachment #391657 - Attachment is obsolete: true
I finally figured out the problem I think - the nsILocalFile result from XRE_GetBinaryPath needs to be normalized. If it isn't - even if it just has "/./" somewhere in the path, startup will fail. Initializing with an FSRef always gives a normalized path since an FSRef is an inode reference, not a path. A CFURL can contain a non-normalized path.
Depends on: 571193
Fixed by the patch on bug 571193.
Status: REOPENED → RESOLVED
Closed: 10 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.