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