mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 62911: Added a RWMutex to libprocess.
Date Wed, 18 Oct 2017 23:33:53 GMT


> On Oct. 17, 2017, 9:56 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/include/process/rwmutex.hpp
> > Lines 28 (patched)
> > <https://reviews.apache.org/r/62911/diff/3/?file=1861094#file1861094line28>
> >
> >     Did a scan of other libraries, interestingly these are named pretty differently:
> >     
> >     c++: shared_mutex (generic terminology, assuming not necessarily used for read/write
pattern)
> >     
> >     go: RWMutex
> >     rust: RWLock
> >     java: ReadWriteLock
> >     
> >     `ReadWriteLock` seems like the best choice for us.
> >     
> >     Also, we should have done this for `process::Mutex` and I realize you're following
the lack of documentation there, but it would be great to document this class, specifically
the priority policy that is currently burned in (it looks like the same notion of fairness
described here: https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html
?).
> 
> Zhitao Li wrote:
>     Ack.
>     
>     Do you think we should also rename `Mutex` to `Lock` for consistency.

Since "lock" doesn't specify what kind of lock, I think the consistent naming there would
be `MutexLock` and `ReadWriteLock`. I'm ok with either `MutexLock` or `Mutex`, since people
generally understand that `Mutex` is a lock, and this is the naming across go, rust, c++.


> On Oct. 17, 2017, 9:56 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/include/process/rwmutex.hpp
> > Lines 50 (patched)
> > <https://reviews.apache.org/r/62911/diff/3/?file=1861094#file1861094line50>
> >
> >     Not your fault since you're just copying, but as an aside, we probably should
have designed process::Mutex::lock to return a `Locked` object that the user can release,
which is safer than having someone else potentially call into unlock accidentally.
> 
> Zhitao Li wrote:
>     I like this design. I'll see how much I can incorporate it in this patch.

I noticed it's similar to what rust provides for its synchronous RWLock, but I don't think
you need to tackle this here if you don't want to since it's pretty subtle. For example, we
can't quite use the RAII design that rust has because it's pretty implicit as to when a last
future reference goes away. If we have a method on the returned locked object, then we have
to crash if it's called twice, whereas the RAII design doesn't have this issue.


> On Oct. 17, 2017, 9:56 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/include/process/rwmutex.hpp
> > Lines 95-96 (patched)
> > <https://reviews.apache.org/r/62911/diff/3/?file=1861094#file1861094line95>
> >
> >     This would probably read a bit easier as:
> >     
> >     ```
> >     if (unlocked) {
> >       grab read lock
> >     } else if (read locked and no waiters) {
> >       grab read lock
> >     } else {
> >       queue
> >     }
> >     ```
> >     
> >     Also it's probably better to explain the priority semantics at the top rather
than here.
> 
> Zhitao Li wrote:
>     Ack for moving the comment into class comment.
>     
>     For the conditions, we would have duplicate on "grab read lock" which I tried to
avoid. Do you think that's more readable?

Well, I just wrote the conditional that way because that's how I reasoned about it when I
wrote the suggested comment about the lock states.

When I read the condition as it is now, I feel like I have to infer from it (i.e. to proceed
it has to be not write locked and no one is waiting, which means that either (1) it's unlocked
or (2) a read is in progress with no one waiting). So the reasoning seems less direct to me
when reading.


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62911/#review188392
-----------------------------------------------------------


On Oct. 17, 2017, 7:56 p.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62911/
> -----------------------------------------------------------
> 
> (Updated Oct. 17, 2017, 7:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Benjamin Mahler, Gilbert Song, and Jason
Lai.
> 
> 
> Bugs: MESOS-8075
>     https://issues.apache.org/jira/browse/MESOS-8075
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The `RWMutex` class is similar to `Mutex`, but allows extra concurrency if
> some actions can be performed concurrently safely while certain actions
> require full mutual exclusive.
> 
> This implementation guarantees starvation free for `lock()` by queuing up
> `rlock()` when some `lock()` is already in queue.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/Makefile.am 94c7a722aab6c36174f117f0b6239cb988e476a9 
>   3rdparty/libprocess/include/process/rwmutex.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/62911/diff/3/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message