Closed
Bug 506812
Opened 16 years ago
Closed 15 years ago
get rid of FSRef usage in XRE_GetBinaryPath
Categories
(Core :: XPCOM, defect, P2)
Tracking
()
RESOLVED
FIXED
People
(Reporter: vlad, Assigned: jaas)
References
(Blocks 1 open bug)
Details
(Whiteboard: [ts])
Attachments
(1 file, 1 obsolete file)
770 bytes,
patch
|
vlad
:
review+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Updated•16 years ago
|
Whiteboard: [ts]
Comment 1•16 years ago
|
||
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.
Reporter | ||
Comment 2•16 years ago
|
||
I don't know if CFURLs are much better, but it's worth a shot.
Comment 3•16 years ago
|
||
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.
Updated•16 years ago
|
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)
Reporter | ||
Updated•16 years ago
|
Attachment #391652 -
Flags: review?(vladimir) → review+
XRE_GetBinaryPath fix pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/4318f781ab87
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)
Reporter | ||
Comment 8•16 years ago
|
||
Comment on attachment 391657 [details] [diff] [review]
nsDirEnumerator fix, v1.0
Also looks good to me..
Attachment #391657 -
Flags: review?(vladimir) → review+
Comment 9•16 years ago
|
||
41% Ts regression on Tiger?
http://graphs.mozilla.org/graph.html#tests=[{%22machine%22:59,%22test%22:16,%22branch%22:1},{%22machine%22:60,%22test%22:16,%22branch%22:1},{%22machine%22:61,%22test%22:16,%22branch%22:1},{%22machine%22:62,%22test%22:16,%22branch%22:1},{%22machine%22:72,%22test%22:16,%22branch%22:1},{%22machine%22:148,%22test%22:16,%22branch%22:1},{%22machine%22:149,%22test%22:16,%22branch%22:1},{%22machine%22:150,%22test%22:16,%22branch%22:1},{%22machine%22:151,%22test%22:16,%22branch%22:1},{%22machine%22:152,%22test%22:16,%22branch%22:1},{%22machine%22:153,%22test%22:16,%22branch%22:1},{%22machine%22:154,%22test%22:16,%22branch%22:1},{%22machine%22:155,%22test%22:16,%22branch%22:1},{%22machine%22:156,%22test%22:16,%22branch%22:1},{%22machine%22:157,%22test%22:16,%22branch%22:1},{%22machine%22:158,%22test%22:16,%22branch%22:1},{%22machine%22:159,%22test%22:16,%22branch%22:1},{%22machine%22:160,%22test%22:16,%22branch%22:1},{%22machine%22:161,%22test%22:16,%22branch%22:1},{%22machine%22:162,%22test%22:16,%22branch%22:1}]&sel=1248892800,1249065600
for http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=358af1196dc2&tochange=59a18d5919a6
Comment 10•16 years ago
|
||
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.)
Assignee | ||
Comment 11•16 years ago
|
||
What Ted said. It is *really* unlikely.
Assignee | ||
Comment 12•16 years ago
|
||
pushed nsDirEnumerator fix to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/ad9a4a3a5409
Assignee | ||
Comment 13•16 years ago
|
||
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: 16 years ago
Resolution: --- → FIXED
Summary: Get rid of remaining MacOS X FSRef uses → Get rid of FSRef usage in XRE_GetBinaryPath and nsDirEnumerator
Comment 14•16 years ago
|
||
Should file a new bug on nsXREDirProvider::GetUserDataDirectory[Home]
Assignee | ||
Comment 15•16 years ago
|
||
I backed out both changesets here. The first to investigate the Ts change, the second due to talos crashes.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 16•16 years ago
|
||
Backing out "XRE_GetBinaryPath fix, v1.0" looks to have fix the Ts regression. Amazing and scary.
Comment 17•15 years ago
|
||
Joel, another Mac-specific startup issue for you to check out.
Reporter | ||
Comment 18•15 years ago
|
||
Cc'ing the right joelr :)
Comment 19•15 years ago
|
||
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
Reporter | ||
Comment 20•15 years ago
|
||
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.
Comment 21•15 years ago
|
||
Why not push the changes and see if they cause a regression on Leopard?
Assignee | ||
Comment 22•15 years ago
|
||
I pushed the XRE_GetBinaryPath fix again.
http://hg.mozilla.org/mozilla-central/rev/845b625b5eb5
Assignee | ||
Comment 23•15 years ago
|
||
Backed out due to talos orange. IIRC this didn't happen last time.
http://hg.mozilla.org/mozilla-central/rev/c1e5dcec20dd
Comment 24•15 years ago
|
||
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).)
Comment 25•15 years ago
|
||
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?
Updated•15 years ago
|
Flags: blocking1.9.2? → blocking1.9.2-
Comment 26•15 years ago
|
||
What's next on this? My Talos does not crash and performance is no worse.
Reporter | ||
Comment 27•15 years ago
|
||
I'd say let's try landing it again; even if there's no performance change, the code gets simpler.
Comment 28•15 years ago
|
||
I can't land anything, though, and Josh is busy. Vlad, can you land this?
Assignee | ||
Comment 29•15 years ago
|
||
Here we go again. Pushed "XRE_GetBinaryPath fix, v1.0" to mozilla-central.
http://hg.mozilla.org/mozilla-central/rev/65aff8912e7c
Assignee | ||
Comment 30•15 years ago
|
||
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.
Assignee | ||
Comment 31•15 years ago
|
||
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
Assignee | ||
Comment 32•15 years ago
|
||
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
Assignee | ||
Comment 33•15 years ago
|
||
I can reproduce the "XRE_GetBinaryPath fix, v1.0" talos crash locally, the crash only happens on 10.5, not 10.6.
Attachment #391657 -
Attachment is obsolete: true
Assignee | ||
Comment 34•15 years ago
|
||
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.
Assignee | ||
Comment 35•15 years ago
|
||
Fixed by the patch on bug 571193.
Status: REOPENED → RESOLVED
Closed: 16 years ago → 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•