----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62911/#review188392 -----------------------------------------------------------
3rdparty/libprocess/include/process/rwmutex.hpp Lines 28 (patched) <https://reviews.apache.org/r/62911/#comment265376> 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 ?). 3rdparty/libprocess/include/process/rwmutex.hpp Lines 33 (patched) <https://reviews.apache.org/r/62911/#comment265378> How about write_lock, write_unlock, read_lock, read_unlock? 3rdparty/libprocess/include/process/rwmutex.hpp Lines 50 (patched) <https://reviews.apache.org/r/62911/#comment265379> 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. 3rdparty/libprocess/include/process/rwmutex.hpp Lines 58 (patched) <https://reviews.apache.org/r/62911/#comment265384> Don't want to check both? ``` CHECK(data->write_locked); CHECK_EQ(data->read_locked, 0u); ``` 3rdparty/libprocess/include/process/rwmutex.hpp Lines 61-73 (patched) <https://reviews.apache.org/r/62911/#comment265386> Maybe a switch with READ/WRITE will be clearer than this if/else? ``` switch (data->waiters.front()->mode) { case READ: // Dequeue the group of readers at the front. while (!data->waiters.empty() && data->waiters.front()->mode == READ) { unblocked.push(data->waiters.front()); data->waiters.pop(); } data->read_locked = unblocked.size(); data->write_locked = false; break; case WRITE: unblocked.push(data->waiters.front()); data->waiters.pop(); CHECK(data->write_locked); CHECK_EQ(data->read_locked, 0u); break; } ``` 3rdparty/libprocess/include/process/rwmutex.hpp Lines 69 (patched) <https://reviews.apache.org/r/62911/#comment265383> You need to set write_locked = false here? 3rdparty/libprocess/include/process/rwmutex.hpp Lines 75-77 (patched) <https://reviews.apache.org/r/62911/#comment265388> This is a little hard to follow, maybe move the empty case to the top? 3rdparty/libprocess/include/process/rwmutex.hpp Lines 80 (patched) <https://reviews.apache.org/r/62911/#comment265390> s/(/ (/ 3rdparty/libprocess/include/process/rwmutex.hpp Lines 95-96 (patched) <https://reviews.apache.org/r/62911/#comment265391> 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. 3rdparty/libprocess/include/process/rwmutex.hpp Lines 109-112 (patched) <https://reviews.apache.org/r/62911/#comment265392> Some grammar mistakes and rogue punctuation here? 3rdparty/libprocess/include/process/rwmutex.hpp Lines 117 (patched) <https://reviews.apache.org/r/62911/#comment265393> CHECK_GT? 3rdparty/libprocess/include/process/rwmutex.hpp Lines 118 (patched) <https://reviews.apache.org/r/62911/#comment265394> add newline 3rdparty/libprocess/include/process/rwmutex.hpp Lines 122 (patched) <https://reviews.apache.org/r/62911/#comment265395> CHECK_EQ 3rdparty/libprocess/include/process/rwmutex.hpp Lines 139-146 (patched) <https://reviews.apache.org/r/62911/#comment265380> I think you could just do: ``` struct Waiter { enum { READ, WRITE } type; Promise<Nothing> promise; } Waiter w = { READ }; data->waiters.push(std::move(w)); ``` 3rdparty/libprocess/include/process/rwmutex.hpp Lines 161-168 (patched) <https://reviews.apache.org/r/62911/#comment265381> ``` // The state lock can be either: // (1) Unlocked: an incoming read or write grabs the lock. // // (2) Read locked (by one or more readers): an incoming write // will queue in the waiters. An incoming read will proceed // if no one is waiting, otherwise it will queue. // // (3) Write locked: incoming reads and writes will queue. size_t read_locked; bool write_locked; std::queue<Waiter> waiters; ``` I don't think we need the Owned, I think we did that in process::Mutex before Promise had move support. - Benjamin Mahler 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 > >
