Add mozilla::StaticMutex, a lazily-initialized mutex suitable for use as a global variable

RESOLVED FIXED in mozilla24

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: justin.lebar+bug, Unassigned)

Tracking

Trunk
mozilla24
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
Hopefully the comments will make it clear what this does.
(Reporter)

Comment 1

6 years ago
Created attachment 751405 [details] [diff] [review]
Part 1, v1: Add mozilla::OffTheBooksMutex, which is just like mozilla::Mutex, except it's not leak-checked.

Delegating the review to khuey.
Attachment #751405 - Flags: review?(khuey)
(Reporter)

Comment 2

6 years ago
Created attachment 751406 [details] [diff] [review]
Part 2, v1: Add mozilla::StaticMutex, a lazily-initialized Mutex suitable for use as a global variable.
Attachment #751406 - Flags: review?(khuey)
(Reporter)

Updated

6 years ago
Blocks: 832609
Comment on attachment 751406 [details] [diff] [review]
Part 2, v1: Add mozilla::StaticMutex, a lazily-initialized Mutex suitable for use as a global variable.

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

::: xpcom/base/StaticMutex.h
@@ +43,5 @@
> +    GetMutex()->Unlock();
> +  }
> +
> +private:
> +  OffTheBooksMutex* GetMutex()

Just Mutex().
Attachment #751406 - Flags: review?(khuey) → review+
I'm not really fond of having different FooMutexAutoLock for every FooMutex, but I'm not sure what the best way to fix that is.
(Reporter)

Comment 5

6 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #4)
> I'm not really fond of having different FooMutexAutoLock for every FooMutex,
> but I'm not sure what the best way to fix that is.

Me either, on both points.

I can do without FooMutexAutoLock if you let me use virtual functions.  But I'm afraid of putting a virtual call in the way of MutexAutoLock, since we use it all over the place.  (Maybe it doesn't matter; it could be that a virtual function is always much cheaper than a mutex operation.  I don't know.)
(Reporter)

Comment 6

6 years ago
Comment on attachment 751406 [details] [diff] [review]
Part 2, v1: Add mozilla::StaticMutex, a lazily-initialized Mutex suitable for use as a global variable.

Let's ask someone who's good at templates.

Ehsan, can you think of a way to do

  MutexAutoLock lock(mutex);

or something similarly concise and have that work if mutex is a mozilla::Mutex
or a mozilla::StaticMutex?  I have two options here, but you may have a better idea.

When I suggested that we could use virtual functions to solve this problem,
here's a sketch of what I meant:

  class MutexAutoLock
  {
    template<class T>
    MutexAutoLock(T* mutex)
    {
      mUnlocker = new Unlocker<T>(mutex);
    }

    ~MutexAutoLock()
    {
      mUnlocker->Unlock();
    }

    class UnlockerBase
    {
      virtual Unlock() = 0;
    };

    template<class T>
    class Unlocker : UnlockerBase
    {
      Unlocker(T* mutex)
        : mMutex(mutex)
      {}

      virtual Unlock()
      {
        mMutex->Unlock();
      }

      T *mMutex;
    };


    nsAutoPtr<UnlockerBase> mUnlocker;
  };

We could probably avoid the call to new with judicious use of placement new
and the knowledge that all Unlocker instances have the same layout, so I'd
hazard to guess that the main overhead of this compared to what we do now would
be the virtual function call.

A less-elegant but perhaps faster alternative would be to hardcode the list of
acceptable mutexes, and essentially do the virtual dispatch ourselves:

  class MutexAutoLock
  {
    MutexAutoLock(Mutex* mutex)
      mMutex(mutex),
      mType(TYPE_MUTEX)
    {}

    MutexAutoLock(StaticMutex* mutex)
      mMutex(mutex),
      mType(TYPE_STATIC_MUTEX)
    {}

    ~MutexAutoLock()
    {
      switch(mType) {
      TYPE_MUTEX:
        reinterpret_cast<Mutex*>(mMutex)->Unlock();
	break;
      TYPE_STATIC_MUTEX:
        reinterpret_cast<StaticMutex*>(mMutex)->Unlock();
	break;
      }
    }

    void* mMutex;
    enum MutexType {
      TYPE_MUTEX,
      TYPE_STATIC_MUTEX
    } mType;
  };

Now that I look at this, I'd guess that the compiler has a decent chance of
optimizing out the switch statement in the destructor, and at that point
there's no reason to keep mType around at all.  So maybe this is the better
option.

Or we can keep things as they are and use StaticMutexAutoUnlock.  :shrug:
Attachment #751406 - Flags: feedback?(ehsan)
(In reply to Justin Lebar [:jlebar] from comment #6)
> Comment on attachment 751406 [details] [diff] [review]
> Part 2, v1: Add mozilla::StaticMutex, a lazily-initialized Mutex suitable
> for use as a global variable.
> 
> Let's ask someone who's good at templates.
> 
> Ehsan, can you think of a way to do
> 
>   MutexAutoLock lock(mutex);
> 
> or something similarly concise and have that work if mutex is a
> mozilla::Mutex
> or a mozilla::StaticMutex?

It looks like MutexAutoLock is already a typedef of a templated class, BaseAutoLock.  Can't you either:

1) Just do |template<typename T> class MutexAutoLock : public BaseAutoLock<T> {};|; or
2) Rename BaseAutoLock to MutexAutoLock and let the compiler figure things out?  The compiler will yell if the argument doesn't support Lock and Unlock methods.
(Reporter)

Comment 8

6 years ago
I think the goal is to avoid typing the name of the class being locked, in either a template param or the name of the class.  Wouldn't I have to do that with both options (1) and (2)?
(In reply to Justin Lebar [:jlebar] from comment #8)
> I think the goal is to avoid typing the name of the class being locked, in
> either a template param or the name of the class.  Wouldn't I have to do
> that with both options (1) and (2)?

Oh, duh.  Right.

I'd guess your second option, with the switching, is more likely to be optimized by current compilers, but some current and most future compilers would be able to optimize the first option with virtual functions.  But it ought to be possible to experiment and see what happens.

Comment 10

6 years ago
What you really want here is for MutexAutoLock be a template on the mutex type, and for the compiler to deduce the type from the argument to the ctor, which is not possible in C++.  In practice I don't see any way to do this without dynamic dispatch.  Your second approach is probably a bit more efficient, but you need to make sure that the compilers we use will actually optimize away the dynamic dispatch.

If I were you, however, I would just use StaticMutexAutoUnlock.  I don't think that this problem is worth fixing given the solutions at hand!

Updated

6 years ago
Attachment #751406 - Flags: feedback?(ehsan) → feedback+
Hm, won't we get into trouble with this since PR_NewLock/PR_DestroyLock will be called perhaps before/after libxul is loaded/unloaded?
(Reporter)

Comment 12

6 years ago
(In reply to ben turner [:bent] from comment #11)
> Hm, won't we get into trouble with this since PR_NewLock/PR_DestroyLock will
> be called perhaps before/after libxul is loaded/unloaded?

We won't call PR_NewLock until the lock is acquired, so unless this class is used outside libxul, I don't think we'd have this problem.  We don't call PR_Unlock at exit, so I don't think we have that problem, either?
(Reporter)

Comment 13

6 years ago
> We won't call PR_NewLock until the lock is acquired,

Because we don't create the mozilla::Mutex until the lock is first acquired.
Oh, I see, StaticMutex doesn't run the OffTheBooksMutex destructor. That sounds fine then.

I bet Valgrind will flag it...
(Reporter)

Comment 15

6 years ago
> I bet Valgrind will flag it...

As "still reachable", I'd expect, which is what it is...
(Reporter)

Comment 16

6 years ago
I think we probably could delete the mutex on shutdown.  You'd just have to

1. lock it
2. set the static reference to nullptr
3. unlock it
4. delete it

But then we make shutdown block on locking the mutex, all for the purpose of "fixing" a "leak".  That doesn't seem worthwhile to me.
Yeah, we could just annotate it away.
https://hg.mozilla.org/mozilla-central/rev/bfe1d6ce6f3e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.