1353787 - use first-fit mutexes on OS X

Status: RESOLVED DUPLICATE of bug 1665411

(Core :: mozglue, enhancement, P5)

Description

[Nathan Froyd [:froydnj]]

Changing the fairness policy makes OS X mutexes roughly an order of
magnitude faster in the contended case.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: use first-fit mutexes on OS X

Comment 2

[Eric Rahm [: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

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: use first-fit mutexes on OS X

Revised patch.

Comment 4

[Eric Rahm [:erahm]]

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?

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e824f50f5ca6
use first-fit mutexes on OS X; r=erahm

Comment 6

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ca1b0650836
Backout e824f50f5ca6 for massive OS X test bustage on a CLOSED TREE

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Pulsebot from comment #6)
> Backout by [e-mail]:
> 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.

Comment 8

[Jon Coppeard (:jonco)]

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%.

Comment 9

[Jon Coppeard (:jonco)]

(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

Comment 10

[Jon Coppeard (:jonco)]

(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.

Comment 11

[Nathan Froyd [:froydnj]]

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?

Comment 12

[Mike Hommey [:glandium]]

Somebody was me, but I was talking about os_unfair_lock, which is 10.12+.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:glandium, could you have a look please?
For more information, please visit auto_nag documentation.