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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Justin Lebar (not reading bugmail)]

Hopefully the comments will make it clear what this does.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v1: Add mozilla::OffTheBooksMutex, which is just like mozilla::Mutex, except it's not leak-checked.

Delegating the review to khuey.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Part 2, v1: Add mozilla::StaticMutex, a lazily-initialized Mutex suitable for use as a global variable.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 5

[Justin Lebar (not reading bugmail)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.)

Comment 6

[Justin Lebar (not reading bugmail)]

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:

Comment 7

[Nathan Froyd [:froydnj]]

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

Comment 8

[Justin Lebar (not reading bugmail)]

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)?

Comment 9

[Nathan Froyd [:froydnj]]

(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

[(no longer active)]

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!

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hm, won't we get into trouble with this since PR_NewLock/PR_DestroyLock will be called perhaps before/after libxul is loaded/unloaded?

Comment 12

[Justin Lebar (not reading bugmail)]

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

Comment 13

[Justin Lebar (not reading bugmail)]

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

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Oh, I see, StaticMutex doesn't run the OffTheBooksMutex destructor. That sounds fine then.

I bet Valgrind will flag it...

Comment 15

[Justin Lebar (not reading bugmail)]

> I bet Valgrind will flag it...

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

Comment 16

[Justin Lebar (not reading bugmail)]

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.

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yeah, we could just annotate it away.

Comment 18

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfe1d6ce6f3e
https://hg.mozilla.org/integration/mozilla-inbound/rev/56f89cd46261

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bfe1d6ce6f3e