1312086 - move js's Mutex and ConditionVariable to mozglue

Status: RESOLVED FIXED

(Core :: mozglue, defect, P3)

Description

[Nathan Froyd [:froydnj]]

We want to use these locks in mozilla::Mutex et al, so it would be convenient if they were in mozglue.

Comment 1

[Mike Hommey [:glandium]]

Related: bug 936843

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 1 - pull the TimeStamp'd ConditionVariable::wait_for into platform-independent code

The implementation is exactly the same for both the Windows and Posix
implementations.  There's no need for the code duplication.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 2 - separate out a ConditionVariableImpl class

For moving the platform-specific bits of Mutex and ConditionVariable to
mozglue, we'd rather not have to bring along things like js::LockGuard.
To do that, we need to separate out the platform-specific bits of
ConditionVariable into a ConditionVariableImpl class, similar to what's
already done for Mutex.  Although we won't have type-checking to ensure
that we pass in locked mutexes for the wait* methods, we expect
higher-level implementations to take care of those details.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 3 - move js::{Mutex,ConditionVariable}Impl to mozglue

This compiles locally for me; currently running it through try.  Mostly a
straightforward cut-and-paste from js/ -> mozglue/.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8832230 [details] [diff] [review]
part 1 - pull the TimeStamp'd ConditionVariable::wait_for into platform-independent code

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

Yep

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> we'd rather not have to bring along things like js::LockGuard.

Wait -- why not?

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8832231 [details] [diff] [review]
part 2 - separate out a ConditionVariableImpl class

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

Not convinced that we shouldn't lift js::LockGuard and unify it with whatever gecko lock guards, but (a) I don't think we need to block (heh) no it, and (b) I'm not the one doing the work, so...

This looks straightforward to me, but I'd like my concern below addressed.

Thanks!

::: js/src/threading/ConditionVariable.h
@@ +68,5 @@
> +
> +template <typename T> using UniqueLock = LockGuard<T>;
> +
> +// A poly-fill for std::condition_variable.
> +class ConditionVariable : public detail::ConditionVariableImpl

I don't think we want public inheritance here. Especially since ConditionVariableImpl will have a less strongly typed interface than ConditionVariable.

In fact, why inherit at all? I think an `Impl impl` member is a better fit here.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8832232 [details] [diff] [review]
part 3 - move js::{Mutex,ConditionVariable}Impl to mozglue

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

It's too bad that we aren't lifting the lock ordering assertions to mozglue as well. I understand that extending the ordering for ALL the gecko locks would be difficult, but we could add a UNCHECKED_LOCK_ORDERING variant that doesn't check/assert, and then as we find deadlocks in practice, make the locks involved in the deadlock into hard assertions. Ah well.

Anyways, regarding inheritance in this patch and the last: r=me with it, I didn't realize it was already like that. Although, if you want to remove it, that is double plus good ;)

Thanks!

::: js/src/threading/ConditionVariable.h
@@ +30,4 @@
>  template <typename T> using UniqueLock = LockGuard<T>;
>  
>  // A poly-fill for std::condition_variable.
> +class ConditionVariable : public mozilla::detail::ConditionVariableImpl

Ditto regarding inheritance.

::: js/src/threading/Mutex.h
@@ +27,5 @@
>  };
>  
>  #ifndef DEBUG
>  
> +class Mutex : public mozilla::detail::MutexImpl

For the record, I don't like this inheritance either, although I see now that it was already set up like this.

@@ +32,3 @@
>  {
>  public:
>    static bool Init() { return true; }

I think this is left over from the removal of boxing the pthread mutex, right? I should have caught it before (sorry!), but since this is now a no-op, could you remove it and calls to it? Thanks!

@@ +46,5 @@
>  // locking order is observed.
>  //
>  // The class maintains a per-thread stack of currently-held mutexes to enable it
>  // to check this.
> +class Mutex : public mozilla::detail::MutexImpl

:-/

::: mozglue/misc/ConditionVariable.h
@@ +60,5 @@
> +#else
> +  void* platformData_[4];
> +#endif
> +};
> +  

Nit: trailing whitespace

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8832231 [details] [diff] [review]
part 2 - separate out a ConditionVariableImpl class

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

::: js/src/threading/ConditionVariable.h
@@ -28,5 @@
>    Timeout
>  };
>  
> -// A poly-fill for std::condition_variable.
> -class ConditionVariable

Wait, no this one was not already using inheritance, so let's not introduce it!

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #6)
> (In reply to Nathan Froyd [:froydnj] from comment #3)
> > we'd rather not have to bring along things like js::LockGuard.
> 
> Wait -- why not?

I hand-waved on #memshrink yesterday, but the better answer goes something like this.

Let's say you require LockGuard<MutexImpl>& parameters in the appropriate condition variable methods.  Remember that these are on detail:: classes, so users are not intended to use them directly.  How do they get used in practice?

1. Things inherit from detail::MutexImpl (Mutex) and detail::ConditionVariable (CV).  So the user generates objects of type LockGuard<Mutex>.  Going from LockGuard<Mutex> to LockGuard<MutexImpl> to use the detail::ConditionVariable methods is doable, but requires some dodgy reinterpret_casts somewhere along the line.  That's not very nice.

2. Things embed detail::MutexImpl (Mutex) and detail::ConditionVariable (CV).  Again, the user generates objects of type LockGuard<Mutex>.  Now there's no nice way of going from LockGuard<Mutex> to LockGuard<MutexImpl>...unless you lay out your objects such that detail::MutexImpl members are at offset 0 in Mutex objects, and then you still have to use reinterpret_cast.  This is arguably worse than the first method.

3) Maybe the problem is that we're expecting the user to construct LockGuard<Mutex> objects directly.  We could instead require the user to expose LockGuard<detail::MutexImpl> objects like so:

Mutex mutex; // inherits or embeds, doesn't matter
...
LockGuard<detail::MutexImpl> g = mutex.lock();

but I would argue that's not very C++-y and users shouldn't be regularly declaring things in detail:: anyway.  Maybe you could argue that people should just use `auto` here, which I guess is a defensible position.  You could make things more C++-y by doing:

class MutexAutoLock
{
public:
  MutexAutoLock(Mutex& m) : guard_(m.lock()) {}
  ~MutexAutoLock() = default;

private:
  LockGuard<detail::MutexImpl> guard_;
};

but now you have to write RAII wrappers for an RAII class, which seems kind of ridiculous.

We could go with option 3--which might still contain some pitfalls that I haven't thought of yet--but that would require changing a lot of things in JS as well as extensive changes to Gecko when XPCOM's locks are converted to use the code in mozglue.  While I am in favor of more Rust-y APIs in Gecko's C++, I think introducing option 3 would not an appropriate Rust-y API at this time.

Have I convinced you? :)

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I guess I was hoping that we would make mozilla::detail::MutexImpl be an implementation detail of mozilla::Mutex, which would be the One True Mutex Class and we wouldn't have issues with wanting to use LockGuards with different Mutex classes because there would only be the One True Mutex Class.

I guess that is naive of me :-/

r=mutex fatigue

Comment 12

[Nathan Froyd [:froydnj]]

I think that would be the ideal state, but I also think that Gecko's Mutex needs (maybe "accumulated cruft" would be a better description) are sufficiently different from JS's Mutex needs that trying to unify them in one class would just make people unhappy.  That would also require dragging in JS engine concepts and Gecko concepts into mozglue (e.g. mutex IDs), which doesn't feel quite right.

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

The mutex IDs and ordering are one of the best things about js::Mutex, but I digress...

Comment 14

[Nathan Froyd [:froydnj]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #8)
> @@ +32,3 @@
> >  {
> >  public:
> >    static bool Init() { return true; }
> 
> I think this is left over from the removal of boxing the pthread mutex,
> right? I should have caught it before (sorry!), but since this is now a
> no-op, could you remove it and calls to it? Thanks!

This is still needed for:

http://dxr.mozilla.org/mozilla-central/source/js/src/vm/Initialization.cpp#103

I guess I could #ifdef DEBUG that call, but...

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Nathan Froyd [:froydnj] from comment #14)
> This is still needed for:
> 
> http://dxr.mozilla.org/mozilla-central/source/js/src/vm/Initialization.
> cpp#103
> 
> I guess I could #ifdef DEBUG that call, but...

Nope, nevermind! Carry on...

Comment 16

[Nathan Froyd [:froydnj]]

Attachment: part 2 - separate out a ConditionVariableImpl class

r+'d by fitzgen in comment 11, with review comments applied.

Comment 17

[Nathan Froyd [:froydnj]]

Attachment: part 3 - move js::{Mutex,ConditionVariable}Impl to mozglue

Here's a version that actually compiles on Windows.  Did not attempt removing
the inheritance from js::Mutex.

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8832639 [details] [diff] [review]
part 3 - move js::{Mutex,ConditionVariable}Impl to mozglue

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

::: js/src/threading/ConditionVariable.h
@@ +95,5 @@
>    // has a minimum granularity of the system's scheduling interval, and may
>    // encounter substantially longer delays, depending on system load.
>    CVStatus wait_for(UniqueLock<Mutex>& lock,
>                      const mozilla::TimeDuration& rel_time) {
> +    return impl_.wait_for(lock.lock, rel_time) == mozilla::detail::CVStatus::Timeout

You could also not change this and allow the conversion from mozilla::detail::CVStatus to js::CVStatus.

::: mozglue/misc/ConditionVariable.h
@@ +25,5 @@
> +  NoTimeout,
> +  Timeout
> +};
> +
> +class ConditionVariableImpl {

I think class MFBT_API ConditionVariableImpl {} would work and avoid having to put MFBT_API on each method.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec0c05239662
part 1 - pull the TimeStamp'd ConditionVariable::wait_for into platform-independent code; r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/655ed7b68ed6
part 2 - separate out a ConditionVariableImpl class; r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0dba8cdf2fc
part 3 - move js::{Mutex,ConditionVariable}Impl to mozglue; r=fitzgen,glandium

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec0c05239662
https://hg.mozilla.org/mozilla-central/rev/655ed7b68ed6
https://hg.mozilla.org/mozilla-central/rev/e0dba8cdf2fc