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