Closed
Bug 1072093
Opened 10 years ago
Closed 10 years ago
Enable the CrossProcessMutex code for use on OS X
Categories
(Core :: IPC, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(2 files, 2 obsolete files)
3.96 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
11.92 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
The CrossProcessMutex is used in the APZ/tiling code for implementing the progressive tiling behavior. Now that we have tiling enabled on OS X by default it would be nice to be able to turn on progressive tiling at the same time as APZ. To do this we need to make sure we use a working version of CrossProcessMutex on OS X, rather than the _unimplemented version we're using now.
Assignee | ||
Comment 1•10 years ago
|
||
green try |
A local build with this seems to work fine. Try push at https://tbpl.mozilla.org/?tree=Try&rev=e73bfc8edaf7 Will request review if that goes green.
Assignee | ||
Comment 2•10 years ago
|
||
This should be good assuming part 1 goes well (which I expect it to).
Attachment #8494241 -
Flags: review?(rbarker)
Assignee | ||
Updated•10 years ago
|
Attachment #8494239 -
Attachment description: Use CrossProcessMutex_posix on OS X → Part 1 - Use CrossProcessMutex_posix on OS X
Comment 3•10 years ago
|
||
Technically for non b2g/e10s we only need a cross-thread (regular) Mutex but doing this can't hurt.
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8494239 [details] [diff] [review] Part 1 - Use CrossProcessMutex_posix on OS X Try looks good!
Attachment #8494239 -
Flags: review?(rbarker)
Attachment #8494239 -
Flags: review?(benjamin)
Comment 5•10 years ago
|
||
Comment on attachment 8494239 [details] [diff] [review] Part 1 - Use CrossProcessMutex_posix on OS X Knowing nothing about CrossProcessMutex other than I wish it didn't exist because it represents the possibility of the chrome process deadlocking on some other process, I'm going to punt this to a mac expert.
Attachment #8494239 -
Flags: review?(benjamin) → review?(smichaud)
Comment 6•10 years ago
|
||
Comment on attachment 8494239 [details] [diff] [review] Part 1 - Use CrossProcessMutex_posix on OS X Review of attachment 8494239 [details] [diff] [review]: ----------------------------------------------------------------- The reason cross process mutexes can not be enabled for MacOS X is that pthread_mutexattr_setpshared is not supported on the platform. Maybe it would be better to have a Mac version that only returns the mutex handle and not a handle to shared memory containing the mutex handle, that way it should work cross thread but would fail if someone tried to use it cross process on a Mac? Maybe add some comments in the header stating it won't actually work cross process on a Mac?
Attachment #8494239 -
Flags: review?(rbarker) → review-
Updated•10 years ago
|
Attachment #8494241 -
Flags: review?(rbarker) → review+
Comment 7•10 years ago
|
||
Comment on attachment 8494239 [details] [diff] [review] Part 1 - Use CrossProcessMutex_posix on OS X I know almost nothing about cross-process mutexes, and I'm currently very busy with something else. Maybe sometime next week? One thing, though: pthread_mutexattr_setpshared *does* seem to be supported on OS X, though it's undocumented there (and doesn't have a man page). On OS X 10.9 (at least), I see an implementation in /usr/lib/system/libsystem_pthread.dylib. Once I have more time, I can check for its presence on other versions of OS X, and see how widely it's used. It's entirely possible that "Apple's own stuff" relies on it, but that they don't want to officially support it for "outsiders". If so, it certainly won't be the first time that's happened :-(
Comment 8•10 years ago
|
||
(In reply to Steven Michaud from comment #7) > Comment on attachment 8494239 [details] [diff] [review] > Part 1 - Use CrossProcessMutex_posix on OS X > > I know almost nothing about cross-process mutexes, and I'm currently very > busy with something else. Maybe sometime next week? > > One thing, though: > > pthread_mutexattr_setpshared *does* seem to be supported on OS X, though > it's undocumented there (and doesn't have a man page). On OS X 10.9 (at > least), I see an implementation in /usr/lib/system/libsystem_pthread.dylib. > > Once I have more time, I can check for its presence on other versions of OS > X, and see how widely it's used. It's entirely possible that "Apple's own > stuff" relies on it, but that they don't want to officially support it for > "outsiders". If so, it certainly won't be the first time that's happened :-( My understanding is pthread_mutexattr_setpshared may have a symbol, but isn't actually implemented. On the man page for pthread_rwlockattr_setpshared it specifically states that: "The PTHREAD_PROCESS_SHARED attribute is not supported." https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man3/pthread_rwlockattr_setpshared.3.html
Comment 9•10 years ago
|
||
The pthread_mutexattr_setpshared implementation I looked at (in assembly code) didn't look like a no-op. And this wouldn't be the first time Apple has "deprecated" something they themselves rely heavily on. I'll still try to find time to check whether Apple's stuff does use this method. But in any case, it will be more difficult to use pthread_mutexattr_setpshared, even if it *is* fully supported, as long as Apple doesn't document exactly how it should be used. For example, Apple's implementation may have Apple-specific quirks.
Comment 10•10 years ago
|
||
(In reply to Steven Michaud from comment #9) > The pthread_mutexattr_setpshared implementation I looked at (in assembly > code) didn't look like a no-op. And this wouldn't be the first time Apple > has "deprecated" something they themselves rely heavily on. > > I'll still try to find time to check whether Apple's stuff does use this > method. > > But in any case, it will be more difficult to use > pthread_mutexattr_setpshared, even if it *is* fully supported, as long as > Apple doesn't document exactly how it should be used. For example, Apple's > implementation may have Apple-specific quirks. Here is the source for 10.8 (looks like some one forgot to include system_pthread in the 10.9 source drops) http://www.opensource.apple.com/source/Libc/Libc-825.40.1/pthreads/pthread_mutex.c /* * Temp: till pshared is fixed correctly */ int pthread_mutexattr_setpshared(pthread_mutexattr_t *attr, int pshared) { #if __DARWIN_UNIX03 if (__unix_conforming == 0) __unix_conforming = 1; #endif /* __DARWIN_UNIX03 */ if (attr->sig == _PTHREAD_MUTEX_ATTR_SIG) { #if __DARWIN_UNIX03 if (( pshared == PTHREAD_PROCESS_PRIVATE) || (pshared == PTHREAD_PROCESS_SHARED)) #else /* __DARWIN_UNIX03 */ if ( pshared == PTHREAD_PROCESS_PRIVATE) #endif /* __DARWIN_UNIX03 */ { attr->pshared = pshared; return (0); } else { return (EINVAL); /* Invalid parameter */ } } else { return (EINVAL); /* Not an initialized 'attribute' structure */ } } That is the only occurrence of PTHREAD_PROCESS_SHARED in the entire file. So it could be implemented in 10.9 but does not appear to be as of 10.8.
Comment 11•10 years ago
|
||
I grepped in the /System/Libraries directory on OS X 10.8.5, and found the following two apps that call pthread_mutexattr_setpshared (imported from /usr/lib/libsystem_c.dylib). (They actually contain methods that call pthread_mutexattr_setpshared.) /System/Library/Frameworks/OpenGL.framework/Libraries/libLLVMContainer.dylib /System/Library/PrivateFrameworks/LLDB.framework/LLDB I don't know what's up with this. But in any case I don't think it counts as "heavy use". So yes, let's just drop the idea of using pthread_mutexattr_setpshared on OS X.
Comment 12•10 years ago
|
||
By the way, I've been using Hopper Disassembler (http://www.hopperapp.com/) to load these apps/libraries and check their implementations and/or use of pthread_mutexattr_setpshared. It's an excellent program! Though unfortunately it's not open source.
Comment 13•10 years ago
|
||
(Following up comment #11) I get the same results on OS X 10.7.5, and the following two users on OS X 10.9.5: /System/Library/Frameworks/OpenGL.framework/Libraries/libLLVMContainer.dylib /System/Library/StagedFrameworks/Safari/JavaScriptCore.framework/Libraries/libllvmForJSC.dylib
Assignee | ||
Comment 14•10 years ago
|
||
The code pasted in comment 10 should return EINVAL in the case where PTHREAD_PROCESS_SHARED is not supported. A return of EINVAL should cause a MOZ_CRASH at http://mxr.mozilla.org/mozilla-central/source/ipc/glue/CrossProcessMutex_posix.cpp?rev=74034d6b6ae6#32 which I didn't see happen when I ran this code. So my bet is that it is implemented, at least on 10.9.4 where I ran it.
Comment 15•10 years ago
|
||
(In reply to comment #14) OK, then, let's keep open the possibility of doing this on OS X. I won't have time for a thorough review before next week. And seeing as we might not be able to fully trust Apple's implementation of pthread_mutexattr_setpshared, I need a fairly thorough set of manual tests I can perform. Someone please provide them. Once we have them, I'll test on all our supported (major) versions of OS X, back to 10.6.
Assignee | ||
Comment 16•10 years ago
|
||
I also wrote a standalone test program to verify that a cross-process mutex works, and indeed it does work fine on OS X 10.9.4. My test program and instructions are at https://gist.github.com/staktrace/44f2a9e55bc4d18d64fb (it may contain errors since I'm pretty unfamiliar with some of these posix APIs).
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Steven Michaud from comment #15) > I need a fairly thorough set of manual tests I > can perform. Someone please provide them. Once we have them, I'll test on > all our supported (major) versions of OS X, back to 10.6. Is the standalone test case I linked to in comment 16 sufficient? I'm not sure if you need a more comprehensive set of tests or not. Also FWIW I'm fine with disabling progressive painting on older versions of OS X that don't support this, if it comes to that.
Comment 18•10 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #16) > I also wrote a standalone test program to verify that a cross-process mutex > works, and indeed it does work fine on OS X 10.9.4. My test program and > instructions are at https://gist.github.com/staktrace/44f2a9e55bc4d18d64fb > (it may contain errors since I'm pretty unfamiliar with some of these posix > APIs). I tested on 10.8.5 and it seems to work. Looking at the source, it appears that 10.4.x contains last version of pthread_mutexattr_setpshared to not support PTHREAD_PROCESS_SHARED. The function returns an error when PTHREAD_PROCESS_SHARED is passed in. Having said that, the fact that it isn't documented and the comment in the code calls it a temp fix (for the past four or five major releases) makes me nervous using it.
Assignee | ||
Comment 19•10 years ago
|
||
I ran my test app on a OS X machine running 10.6.8 and the call to setpshared failed :(
Assignee | ||
Comment 20•10 years ago
|
||
Do either of you have access to a machine running 10.7 where we can test if this works? Also, are there any other alternatives for cross-process mutexes that will work on all our supported OS X versions? I'm not really sure what the next step here is if we want to have a CrossProcessMutex implementation on OS X.
Comment 21•10 years ago
|
||
I can test on all major versions of OS X back to 10.6.8. But right now I simply don't have the time. I hope that by next week things will be a little better.
Assignee | ||
Comment 22•10 years ago
|
||
Sounds good, thanks.
Comment 23•10 years ago
|
||
I've finally got some time to spend on this.
> I ran my test app on a OS X machine running 10.6.8 and the call to setpshared failed :(
Actually it seems to work fine, if you first play a sly trick. You need to set the TET_EXECUTE environment variable before you call the "creator"! E.g. TET_EXECUTE=1 ./a.out foo.
(I found this out by looking at the machine code for pthread_mutexattr_setpshared on 10.6.8.)
I'm not entirely sure what the significance of this is. Google does find a number of hits for TET_EXECUTE, none of them having to do with Apple. But I expect Apple's using it as a way to limit access to pthread_mutexattr_setpshared to it's "own" apps.
Comment 24•10 years ago
|
||
Kartikaya, your testcase from comment #16 works just fine on OS X 10.7.5, without the TET_EXECUTE trick.
Comment 25•10 years ago
|
||
That testcase also works fine on OS X 10.8.5, 10.9.5 and 10.10.
Comment 26•10 years ago
|
||
Comment on attachment 8494239 [details] [diff] [review] Part 1 - Use CrossProcessMutex_posix on OS X I'm fine with this, in principle. We may, of course, later discover problems that will make it difficult or even impossible to use cross process mutexes on OS X. But I don't think that's likely, and we can cross (or burn) that bridge when we come to it. I don't know how important it is to support cross process mutexes on OS X 10.6. I suspect the answer is "not very", even though a fifth to a quarter of our users are still on that version of OS X. Whether you support it or not, though, you'll need to add code that determines the OS X version at runtime, and then act accordingly -- either by setting the TET_EXECUTE environment variable or by somehow indicating that cross process mutexes aren't supported. The methods you should use to determine the OS X version are here: https://hg.mozilla.org/mozilla-central/annotate/04a87b6ff211/widget/cocoa/nsCocoaFeatures.h#l11
Attachment #8494239 -
Flags: review?(smichaud) → review+
Comment 27•10 years ago
|
||
I looked a bit harder and found an Apple-related reference to TET_EXECUTE, here: #ifdef PR_5243343 /* 5243343 - temporary hack to detect if we are running the conformance test */ if(getenv("TET_EXECUTE")) PR_5243343_flag = 1; #endif /* PR_5243343 */ http://www.opensource.apple.com/source/Libc/Libc-825.40.1/sys/__libc_init.c?txt This doesn't inspire a whole lot of confidence that pthread_mutexattr_setpshared will work correctly on OS X 10.6 :-(
Assignee | ||
Comment 28•10 years ago
|
||
Thanks for looking into this! I don't really want to propagate the TET_EXECUTE hack so I'll just do a MOZ_CRASH on pre-Lion versions. The code that I want to use this with is the progressive painting code which we can also disable prior to Lion. This makes sense to do anyway because progressive painting doesn't make sense without tiling, and tiling is disabled by default before Lion.
Assignee | ||
Comment 29•10 years ago
|
||
Updated to MOZ_CRASH on OS X 10.6. Carrying r+ from smichaud.
Attachment #8494239 -
Attachment is obsolete: true
Attachment #8515986 -
Flags: review+
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8494241 -
Attachment is obsolete: true
Attachment #8515993 -
Flags: review?(bas)
Assignee | ||
Updated•10 years ago
|
Attachment #8515993 -
Flags: review?(bas) → review?(bgirard)
Updated•10 years ago
|
Attachment #8515993 -
Flags: review?(bgirard) → review+
Assignee | ||
Comment 31•10 years ago
|
||
Thanks! remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/33b7cccd5e2b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6c2fb5df16ba
https://hg.mozilla.org/mozilla-central/rev/33b7cccd5e2b https://hg.mozilla.org/mozilla-central/rev/6c2fb5df16ba
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•