Closed Bug 1312087 Opened 3 years ago Closed 3 years ago

make mozilla::Mutex et al not use PR_* constructs

Categories

(Core :: XPCOM, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(2 files)

This will be glorious when we can do it.
Priority: -- → P3
This or bug 1312086 is a dupe of bug 1097513.
This change moves us away from NSPR primitives for our primary
synchronization primitives.  We're still using PRMonitor for
ReentrantMonitor, however.

The benefits of this change:

* Slightly faster, as we don't have to deal with some of NSPR's overhead;

* Smaller datatypes.  On POSIX platforms in particular, PRLock is
  enormous. PRCondVar also has some unnecessary overhead.

* Less dynamic memory allocation.  Out of necessity, Mutex and CondVar
  allocated the NSPR data structures they needed, which lead to
  unnecessary checks for failure.

  While sizeof(Mutex) and sizeof(CondVar) may get bigger, since they're
  embedding structures now, the total memory usage should be less.

* Less NSPR usage.  This shouldn't need any explanation.
Attachment #8847277 - Flags: review?(erahm)
Depends on: 1273422
I think there are fundamental differences b/w mozglue and NSPR locks that we should discuss before fully switching over.

Posix implementations
=====================

Mutex
-----
* NSPR:
  - |PR_NewLock|
    - Uses PTHREAD_MUTEX_ADAPTIVE_NP if available, otherwise default (not error checking)
  - Unlock checks that the current thread owns the lock, doesn't try to lock, returns failure, we crash in debug, don't bother in release
* mozglue:
  -| mozilla::detail::MutexImpl()|
    - Uses PTHREAD_MUTEX_ERRORCHECK on debug only, otherwise default
  - All calls will MOZ_CRASH on failure, so in release maybe more crashes?
  - Unlock checks the return value, but we're not error checking in release so yolo

> Attempting to unlock the mutex if it was not locked by the calling thread
> results in undefined behaviour. Attempting to unlock the mutex if it is not
> locked results in undefined behaviour. [1]

CondVar
-------
* NSPR:
  - |PR_NewCondVar|
    - Uses default attr
  - |PR_WaitCondVar| is very different, wait is always done as absolute time, returns 0 on timeout
  - |PR_Notify| is very different, asserts on failure in debug, not release
> /*
>  * Notifies just get posted to the protecting mutex. The
>  * actual notification is done when the lock is released so that
>  * MP systems don't contend for a lock that they can't have.
>  */

* mozglue:
  - |mozilla::detail::ConditionVariableImpl()|
    - Uses |pthread_condattr_setclock| to set a monotonic attribute if available
  - All calls release assert on failure (except timeout)
  - |wait_for| does some wonky stuff with clocks

So that's just posix, I think we'd want to look into windows as well.

[1] http://pubs.opengroup.org/onlinepubs/7908799/xsh/pthread_mutex_lock.html
(In reply to Eric Rahm [:erahm] from comment #3)
> I think there are fundamental differences b/w mozglue and NSPR locks that we
> should discuss before fully switching over.
> 
> Posix implementations
> =====================
> 
> Mutex
> -----
> * NSPR:
>   - |PR_NewLock|
>     - Uses PTHREAD_MUTEX_ADAPTIVE_NP if available, otherwise default (not
> error checking)

We can do this in mozglue; it's probably a strict improvement.

>   - Unlock checks that the current thread owns the lock, doesn't try to
> lock, returns failure, we crash in debug, don't bother in release
> * mozglue:
>   -| mozilla::detail::MutexImpl()|
>     - Uses PTHREAD_MUTEX_ERRORCHECK on debug only, otherwise default
>   - All calls will MOZ_CRASH on failure, so in release maybe more crashes?

Maybe.  But if you look at the return values in the spec, the errors that would happen are basically all things we'd want to crash on anyway (EINVAL, EPERM).  I don't think this is a concern.

>   - Unlock checks the return value, but we're not error checking in release
> so yolo

I'm not too worried about this.

> CondVar
> -------
> * NSPR:
>   - |PR_NewCondVar|
>     - Uses default attr
>   - |PR_WaitCondVar| is very different, wait is always done as absolute
> time, returns 0 on timeout

I don't understand this: the wait amount is a relative time, since it's PR_IntervalTime.

>   - |PR_Notify| is very different, asserts on failure in debug, not release

These asserts that you speak of...this is pt_PostNotifyToCvar?  We get essentially the same asserts from the deadlock detector.  And POSIX doesn't specify that you have to have the mutex locked when you notify, either, so those asserts are just extra NSPR checking.

> > /*
> >  * Notifies just get posted to the protecting mutex. The
> >  * actual notification is done when the lock is released so that
> >  * MP systems don't contend for a lock that they can't have.
> >  */

I'm not too worried about this difference.
Alright I'm not particularly concerned on Windows.

I guess the main difference's we should look out for:
  - error checking on posix in debug, possibly more crashes (although in theory deadlock detector should catch these things)
  - release assertions, possibly more crashes (not necessarily a bad thing)
Comment on attachment 8847277 [details] [diff] [review]
move mozilla::{Mutex,CondVar} to use mozglue locking primitives

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

Can you add a patch that updates mozglue to use |PTHREAD_MUTEX_ADAPTIVE_NP| before landing this?

::: xpcom/threads/BlockingResourceBase.cpp
@@ +387,5 @@
>  void
>  OffTheBooksMutex::Unlock()
>  {
> +  Release();
> +  mOwningThread = nullptr;

For parity with nspr can you add an AssertCurrentThreadOwns before unlocking? I'd be happier if it were a release assert, but debug would be good enough I guess.

::: xpcom/threads/Mutex.h
@@ +32,5 @@
>   * OffTheBooksMutex is identical to Mutex, except that OffTheBooksMutex doesn't
>   * include leak checking.  Sometimes you want to intentionally "leak" a mutex
>   * until shutdown; in these cases, OffTheBooksMutex is for you.
>   */
> +class OffTheBooksMutex : public detail::MutexImpl, BlockingResourceBase

I guess this is public for the equality operator?

@@ +60,5 @@
>  
>  #ifndef DEBUG
>    /**
>     * Lock
>     * @see prlock.h

Can get rid of prlock.h refs.
Attachment #8847277 - Flags: review?(erahm) → review+
Comment on attachment 8847277 [details] [diff] [review]
move mozilla::{Mutex,CondVar} to use mozglue locking primitives

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

::: xpcom/threads/CondVar.h
@@ +68,5 @@
> +      mImpl.wait(*mLock);
> +    } else {
> +      mImpl.wait_for(*mLock, TimeDuration::FromMilliseconds(double(aInterval)));
> +    }
> +    return NS_OK;

Oh and as follow up it would be nice to make these functions void returns.
This matches NSPR's behavior for its PRLock type.
Attachment #8849268 - Flags: review?(erahm)
Attachment #8849268 - Flags: review?(erahm) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/85458fefc1ae
part 0 - use PTHREAD_MUTEX_ADAPTIVE_NP mutexes on Linux/glibc; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/54440069e459
part 1 - move mozilla::{Mutex,CondVar} to use mozglue locking primitives; r=erahm
https://hg.mozilla.org/mozilla-central/rev/85458fefc1ae
https://hg.mozilla.org/mozilla-central/rev/54440069e459
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Blocks: 1350955
Duplicate of this bug: 1097513
You need to log in before you can comment on or make changes to this bug.