mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Zhitao Li <zhitaoli...@gmail.com>
Subject Re: Review Request 62911: Added a RWMutex to libprocess.
Date Wed, 18 Oct 2017 21:46:22 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
?).

Ack.

Do you think we should also rename `Mutex` to `Lock` for consistency.


> On Oct. 17, 2017, 9:56 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/include/process/rwmutex.hpp
> > Lines 33 (patched)
> > <https://reviews.apache.org/r/62911/diff/3/?file=1861094#file1861094line33>
> >
> >     How about write_lock, write_unlock, read_lock, read_unlock?

ack. The current names were borrowed from golang which I uses mostly, but your set of names
seem better for `ReadWriteLock`.


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

I like this design. I'll see how much I can incorporate it in this patch.


> On Oct. 17, 2017, 9:56 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/include/process/rwmutex.hpp
> > Lines 58 (patched)
> > <https://reviews.apache.org/r/62911/diff/3/?file=1861094#file1861094line58>
> >
> >     Don't want to check both?
> >     
> >     ```
> >     CHECK(data->write_locked);
> >     CHECK_EQ(data->read_locked, 0u);
> >     ```

Ack.


> On Oct. 17, 2017, 9:56 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/include/process/rwmutex.hpp
> > Lines 75-77 (patched)
> > <https://reviews.apache.org/r/62911/diff/3/?file=1861094#file1861094line75>
> >
> >     This is a little hard to follow, maybe move the empty case to the top?

I'm going to move `data->write_locked = false;` unconditionally right after top level `CHECK`s.
If we should reacquire write lock, reassign true to it.


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

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?


- Zhitao


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