Add a read write lock

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: manishearth, Unassigned)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
Would be useful for Stylo when a worker thread wants to do something like access something cached.
(Reporter)

Updated

a year ago
Blocks: 1362599
Created attachment 8866879 [details] [diff] [review]
add mozilla::RWLock

This sort of lock is mostly useful in the context of Stylo right now,
but perhaps there are other applications waiting to be written.

Design notes:

* I considered trying to implement this more like Rust's RWLock, but decided
  that might be a bridge too far.

* The checking in BlockingResourceBase is very rudimentary.  I did not feel
  like trying to teach DeadlockDetector how to handle shared (read) locks.

* This is in xpcom, rather than mozglue, as I don't think we have any uses in
  JS.  We can move it to mozglue if anything comes up, and putting it in xpcom
  for now elides some boilerplate that mozglue would require.

* There are no tests...I looked at a couple of different places for inspiration
  for tests, but none of them seemed applicable or actual tests that would
  indicate that we got read locks correct, as opposed to getting them mixed up
  with write locks or something.  Ideas welcome.
Attachment #8866879 - Flags: review?(erahm)

Comment 2

a year ago
Comment on attachment 8866879 [details] [diff] [review]
add mozilla::RWLock

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

It would be nice to at least add a sanity test, something that spins up a producer thread (writer) and a few consumer threads (reader).

::: xpcom/threads/BlockingResourceBase.cpp
@@ +406,5 @@
> +void
> +RWLock::ReadLock()
> +{
> +  // All we want to ensure here is that we're not attempting to acquire the
> +  // read lock while this thread is holding the write lock.

This statement doesn't seem quite right, what we're checking below is that a read lock isn't acquired when a write lock is held. It doesn't enforce "this thread doesn't already have a write lock."

I guess what you mean to say is that: "A read lock is not acquired while a write lock is held".

::: xpcom/threads/RWLock.cpp
@@ +10,5 @@
> +#include <windows.h>
> +
> +static_assert(sizeof(SRWLOCK) <= sizeof(void*), "SRWLOCK is too big!");
> +
> +#define RWLOCK (reinterpret_cast<SRWLOCK*>(&mRWLock))

This confused me for a moment, so maybe what you really want is:

> #define RWLOCK (&reinterpret_cast<SRWLOCK>(mRWLock))

@@ +26,5 @@
> +{
> +#ifdef XP_WIN
> +  InitializeSRWLock(RWLOCK);
> +#else
> +  MOZ_RELEASE_ASSERT(pthread_rwlock_init(RWLOCK, nullptr) == 0,

I'm not a big fan of macros that hide access to whatever variable they're working with. Maybe something like |NativeHandle(mRWLock)| would be clearer?

@@ +30,5 @@
> +  MOZ_RELEASE_ASSERT(pthread_rwlock_init(RWLOCK, nullptr) == 0,
> +                     "pthread_rwlock_init failed");
> +#endif
> +}
> +

We need a dtor. For pthread we should call |pthread_rwlock_destroy|. For windows...well it seems to claim you don't need to destroy it, but that sounds crazy. I guess it's just a set of atomically accessed counters in the SRWLOCK struct.

::: xpcom/threads/RWLock.h
@@ +45,5 @@
> +{
> +public:
> +  explicit RWLock(const char* aName);
> +
> +  ~RWLock() = default;

I don't think we want default here...

@@ +65,5 @@
> +  void ReadUnlockInternal();
> +  void WriteLockInternal();
> +  void WriteUnlockInternal();
> +
> +  RWLock();

Can we delete this?

@@ +78,5 @@
> +  void* mRWLock;
> +#endif
> +
> +#ifdef DEBUG
> +  // We record the owning thread for write locks only.

'for write locks only' => 'when acquiring a write lock'? Just kind of sounds like there's a thing called a 'write lock' otherwise.

@@ +92,5 @@
> +  explicit AutoReadLock(RWLock& aLock MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
> +    : mLock(&aLock)
> +  {
> +    MOZ_GUARD_OBJECT_NOTIFIER_INIT;
> +    MOZ_ASSERT(mLock, "null lock");

How can you even pass in a null ref?

@@ +102,5 @@
> +    mLock->ReadUnlock();
> +  }
> +
> + private:
> +  AutoReadLock();

Can we delete this?

@@ +106,5 @@
> +  AutoReadLock();
> +  AutoReadLock(const AutoReadLock&) = delete;
> +  AutoReadLock& operator=(const AutoReadLock&) = delete;
> +
> +  RWLock* mLock;

Why store this as a pointer and not a ref?

@@ +119,5 @@
> +  explicit AutoWriteLock(RWLock& aLock MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
> +    : mLock(&aLock)
> +  {
> +    MOZ_GUARD_OBJECT_NOTIFIER_INIT;
> +    MOZ_ASSERT(mLock, "null lock");

ditto

@@ +129,5 @@
> +    mLock->WriteUnlock();
> +  }
> +
> + private:
> +  AutoWriteLock();

ditto

@@ +133,5 @@
> +  AutoWriteLock();
> +  AutoWriteLock(const AutoWriteLock&) = delete;
> +  AutoWriteLock& operator=(const AutoWriteLock&) = delete;
> +
> +  RWLock* mLock;

ditto
Attachment #8866879 - Flags: review?(erahm) → feedback+
(In reply to Eric Rahm [:erahm] from comment #2)
> ::: xpcom/threads/BlockingResourceBase.cpp
> @@ +406,5 @@
> > +void
> > +RWLock::ReadLock()
> > +{
> > +  // All we want to ensure here is that we're not attempting to acquire the
> > +  // read lock while this thread is holding the write lock.
> 
> This statement doesn't seem quite right, what we're checking below is that a
> read lock isn't acquired when a write lock is held. It doesn't enforce "this
> thread doesn't already have a write lock."
>
> I guess what you mean to say is that: "A read lock is not acquired while a
> write lock is held".

Aren't the comment and your rephrasing exactly the same?  I don't understand how "ensure...not attempting to acquire the read lock while this thread is holding the write lock" and "a read lock is not acquired while a write lock is held" are saying different things.  The latter version is a little less precise, even, because it doesn't talk about which threads are holding what.

My understanding of things is that CheckAcquire would complain if we are found to be acquiring an already held (write) lock.

> ::: xpcom/threads/RWLock.cpp
> @@ +10,5 @@
> > +#include <windows.h>
> > +
> > +static_assert(sizeof(SRWLOCK) <= sizeof(void*), "SRWLOCK is too big!");
> > +
> > +#define RWLOCK (reinterpret_cast<SRWLOCK*>(&mRWLock))
> 
> This confused me for a moment, so maybe what you really want is:
> 
> > #define RWLOCK (&reinterpret_cast<SRWLOCK>(mRWLock))

No, that's not what I want...reinterpret_cast'ing from a pointer to a structure is more confusing than what's there right now, IMHO.

> @@ +26,5 @@
> > +{
> > +#ifdef XP_WIN
> > +  InitializeSRWLock(RWLOCK);
> > +#else
> > +  MOZ_RELEASE_ASSERT(pthread_rwlock_init(RWLOCK, nullptr) == 0,
> 
> I'm not a big fan of macros that hide access to whatever variable they're
> working with. Maybe something like |NativeHandle(mRWLock)| would be clearer?

I guess I could do that.

> @@ +30,5 @@
> > +  MOZ_RELEASE_ASSERT(pthread_rwlock_init(RWLOCK, nullptr) == 0,
> > +                     "pthread_rwlock_init failed");
> > +#endif
> > +}
> > +
> 
> We need a dtor. For pthread we should call |pthread_rwlock_destroy|. For
> windows...well it seems to claim you don't need to destroy it, but that
> sounds crazy. I guess it's just a set of atomically accessed counters in the
> SRWLOCK struct.

Good call.

> @@ +78,5 @@
> > +  void* mRWLock;
> > +#endif
> > +
> > +#ifdef DEBUG
> > +  // We record the owning thread for write locks only.
> 
> 'for write locks only' => 'when acquiring a write lock'? Just kind of sounds
> like there's a thing called a 'write lock' otherwise.

Well, this is a read/write lock, so talking about a write lock seems reasonable.  I can rephrase, I guess.

> @@ +106,5 @@
> > +  AutoReadLock();
> > +  AutoReadLock(const AutoReadLock&) = delete;
> > +  AutoReadLock& operator=(const AutoReadLock&) = delete;
> > +
> > +  RWLock* mLock;
> 
> Why store this as a pointer and not a ref?

For consistency with the way MutexAutoLock and friends work.  I guess we could change it here if you feel strongly about it.
Created attachment 8870149 [details] [diff] [review]
add mozilla::RWLock

Review feedback mostly addressed, a really basic test added.
Attachment #8870149 - Flags: review?(erahm)
Attachment #8866879 - Attachment is obsolete: true

Comment 5

a year ago
Comment on attachment 8870149 [details] [diff] [review]
add mozilla::RWLock

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

Looks good, thanks for adding the sanity test!
Attachment #8870149 - Flags: review?(erahm) → review+

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/04dbb99eec87
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Updated

a year ago
Blocks: 1367619
You need to log in before you can comment on or make changes to this bug.