Wrap CDMCaps in a Rust-style mutex

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

unspecified
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(2 attachments)

Rust's std::sync::Mutex has some nice properties. It's philosphy is to lock data, rather than code.

It wraps data you're trying to make thread safe with a mutex, and in order to get a reference to the wrapped data you need to lock the mutex and access it through an intermediate layer. This is good, as the mutex that's protecting access to the data is explicitly associated with the data, and it's impossible to forget to take the lock before accessing the data.

I had tried to do something like this in CDMCaps, but it was a bit clunky. I'd like to try something like Rust's wrapping mutex here, to see what it feels like.
Comment on attachment 8937901 [details]
Bug 1426291 - Add Rust style mutex template which wraps data with the mutex that synchronizes it.

https://reviewboard.mozilla.org/r/208598/#review214368

::: dom/media/eme/DataMutex.h:37
(Diff revision 1)
> +//    auto& x = a.ref();
> +//    x.AppendElement(1u);
> +//    assert(x[0], 1u);
> +//
> +template<typename T>
> +class DataMutex {

'{' on its own line. Please run clang-format before landing the patch.

::: dom/media/eme/DataMutex.h:40
(Diff revision 1)
> +//
> +template<typename T>
> +class DataMutex {
> +public:
> +
> +  DataMutex(const char* aName)

explicit.

::: dom/media/eme/DataMutex.h:51
(Diff revision 1)
> +    : mMutex(aName)
> +    , mValue(aValue)
> +  {
> +  }
> +
> +  class MOZ_STACK_CLASS AutoLock {

|auto x = u32DataMutex.Lock()| will still compile without making AutoLock a public member.

::: dom/media/eme/DataMutex.h:54
(Diff revision 1)
> +  }
> +
> +  class MOZ_STACK_CLASS AutoLock {
> +  public:
> +
> +    T* operator->() const {

It might be worth to make these accessors callable only on l-values as RefPtr does.

::: dom/media/eme/DataMutex.h:63
(Diff revision 1)
> +    T& operator*() const {
> +      return ref();
> +    }
> +
> +    T& ref() const {
> +      MOZ_ASSERT(!!mOwner);

Just MOZ_ASSERT(mOwner) will do.

::: dom/media/eme/DataMutex.h:92
(Diff revision 1)
> +    {
> +      MOZ_ASSERT(!!mOwner);
> +      mOwner->mMutex.Lock();
> +    }
> +
> +    DataMutex<T>* mOwner = nullptr;

No in-class initialization is needed since we always init the member in the constructors.

::: dom/media/eme/DataMutex.h:96
(Diff revision 1)
> +
> +    DataMutex<T>* mOwner = nullptr;
> +  };
> +
> +  AutoLock Lock() {
> +    return Move(AutoLock(this));

Do |return AutoLock(this)| instead so the compiler has a better chance to do "return value optimization".

::: dom/media/eme/DataMutex.h:100
(Diff revision 1)
> +  AutoLock Lock() {
> +    return Move(AutoLock(this));
> +  }
> +
> +private:
> +  OffTheBooksMutex mMutex;

Any reason to prefer OffTheBooksMutex over Mutex?
Attachment #8937901 - Flags: review?(jwwang) → review+
Comment on attachment 8937902 [details]
Bug 1426291 - Use DataMutex<T> to synchronize CDMCaps.

https://reviewboard.mozilla.org/r/208600/#review214380
Attachment #8937902 - Flags: review?(jwwang) → review+
Thanks for the review!

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #3)
> Comment on attachment 8937901 [details]
> Bug 1426291 - Add Rust style mutex template which wraps data with the mutex
> that synchronizes it.
> 
[...]
> ::: dom/media/eme/DataMutex.h:100
> (Diff revision 1)
> > +  AutoLock Lock() {
> > +    return Move(AutoLock(this));
> > +  }
> > +
> > +private:
> > +  OffTheBooksMutex mMutex;
> 
> Any reason to prefer OffTheBooksMutex over Mutex?

No. I'll change this, and address the other comments.
Comment on attachment 8937901 [details]
Bug 1426291 - Add Rust style mutex template which wraps data with the mutex that synchronizes it.

https://reviewboard.mozilla.org/r/208598/#review214390

::: commit-message-2d580:3
(Diff revision 2)
> +Bug 1426291 - Add Rust style mutex template which wraps data with the mutex that synchronizes it. r=jwwang
> +
> +Rust's std::sync::Mutex has some nice properties. It's philosphy is to lock

OCD fly-by: "It's" -> "Its".
For later reference: https://searchfox.org/mozilla-central/source/js/src/threading/ExclusiveData.h
And I have my own attempt (before I knew about the above) somewhere on Try. History always repeats itself. :-P
Pushed by cpearce@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4ae5f2caa471
Add Rust style mutex template which wraps data with the mutex that synchronizes it. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/fb96226aef90
Use DataMutex<T> to synchronize CDMCaps. r=jwwang
https://hg.mozilla.org/mozilla-central/rev/4ae5f2caa471
https://hg.mozilla.org/mozilla-central/rev/fb96226aef90
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.