Open Bug 1353787 Opened 4 years ago Updated 2 years ago

use first-fit mutexes on OS X

Categories

(Core :: mozglue, enhancement, P5)

enhancement

Tracking

()

People

(Reporter: froydnj, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

(Whiteboard: [stylo])

Attachments

(1 file, 1 obsolete file)

Changing the fairness policy makes OS X mutexes roughly an order of
magnitude faster in the contended case.
Attached patch use first-fit mutexes on OS X (obsolete) — Splinter Review
Attachment #8854955 - Flags: review?(erahm)
Comment on attachment 8854955 [details] [diff] [review]
use first-fit mutexes on OS X

Review of attachment 8854955 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, we should probably do some talos runs before landing to make sure the microbenchmarks hold up.

::: mozglue/misc/Mutex_posix.cpp
@@ +31,5 @@
>  #endif
>  
>  #if defined(DEBUG)
>  #define ATTR_REQUIRED
> +#define ATTR_SETTYPE

We still probably want ATTR_SETPOLICY for the OSX case. Our debug builds are slow enough :(

@@ +36,3 @@
>  #define MUTEX_KIND PTHREAD_MUTEX_ERRORCHECK
>  #elif defined(ADAPTIVE_MUTEX_SUPPORTED)
>  #define ATTR_REQUIRED

This is getting a bit unruly, I wonder if we could ditch ATTR_REQUIRED and change the check below to:

> #if defined(ATTR_SETTYPE) || defined(ATTR_SETPOLICY)

Also we could simplify a bit and:

> #define ATTR_SETTYPE PTHREAD_MUTEX_ADAPTIVE_NP
> ...
> #define ATTR_SETPOLICY _PTHREAD_MUTEX_POLICY_FIRSTFIT

@@ +42,5 @@
> +// OS X's mutexes are fair by default, which means they can be rather
> +// slow in the contended case.  OS X 10.7 and above provides an OS
> +// X-only function to set the mutex fairness policy, which makes mutexes
> +// non-fair (i.e. like every other platform we support) and increases
> +// performance in the contended case by an order of magnitude or so.

Nice comment.

@@ +63,3 @@
>    TRY_CALL_PTHREADS(pthread_mutexattr_settype(&attr, MUTEX_KIND),
>                      "mozilla::detail::MutexImpl::MutexImpl: pthread_mutexattr_settype failed");
> +#elif defined(ATTR_SETPOLICY)

... and these could be separate if defined's

@@ +65,5 @@
> +#elif defined(ATTR_SETPOLICY)
> +  TRY_CALL_PTHREADS(pthread_mutexattr_setpolicy_np(&attr, _PTHREAD_MUTEX_POLICY_FIRSTFIT),
> +                    "mozilla::detail::MutexImpl::MutexImpl: pthread_mutex_setpolicy_np failed");
> +#else
> +#error mutex attribute required, but nothing to do

... and then this check wouldn't be necessary
Attachment #8854955 - Flags: review?(erahm) → feedback+
Revised patch.
Attachment #8855907 - Flags: review?(erahm)
Attachment #8854955 - Attachment is obsolete: true
Comment on attachment 8855907 [details] [diff] [review]
use first-fit mutexes on OS X

Review of attachment 8855907 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks good. Did you end up doing any talos runs?
Attachment #8855907 - Flags: review?(erahm) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e824f50f5ca6
use first-fit mutexes on OS X; r=erahm
Backout by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ca1b0650836
Backout e824f50f5ca6 for massive OS X test bustage on a CLOSED TREE
(In reply to Pulsebot from comment #6)
> Backout by nfroyd@mozilla.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5ca1b0650836
> Backout e824f50f5ca6 for massive OS X test bustage on a CLOSED TREE

I haven't debugged this on a mac, but from the treeherder failures, it looks like these kinds of mutexes cannot be used (reliably?) with condition variables.  Boo!

So, we have two options:

1) Abandon this whole idea.
2) Introduce a new mutex type, WaitableMutex or somesuch, which would be used in the implementation of Monitor and available for people who *really* needed to use CondVar on its own.  All other Mutexes would unusable with CondVar and would use this style of mutex under the hood.

I'm disinclined to do the second, because it would add quite some complexity to things.  It would be an obvious win for contended mutexes, but I don't really know how many contended mutexes we have in Gecko, so the payoff is not obvious.
Whiteboard: [stylo]
I've been doing some benchmarking for parallel JS parsing and found that performance with a heavily contented mutex on OS X is really bad.  With a pathalogical testcase intended to produce lock contention the total time taken increases by over four times when adding more threads on OSX, whereas on linux it decreases by ~20%.
(In reply to Nathan Froyd [:froydnj] (out until April 30) from comment #7)
> I haven't debugged this on a mac, but from the treeherder failures, it looks
> like these kinds of mutexes cannot be used (reliably?) with condition
> variables.  Boo!

I tried using this for a single mutex in the JS engine that isn't used with condition variables, and I still saw hangs waiting on the mutex, e.g.:

https://treeherder.mozilla.org/logviewer.html#?job_id=175782219&repo=try&lineNumber=5454
(In reply to Jon Coppeard (:jonco) from comment #9)
I couldn't figure out how to make this API work so I spilt off bug 1457882 to try a different approach.
Somebody mentioned that this API is only available on newer OS X versions, and our Mac machines on treeherder do not support the newer API?  Perhaps we might be able to do some sort of runtime test, if this is true?
Somebody was me, but I was talking about os_unfair_lock, which is 10.12+.
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.