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 Tue, 17 Oct 2017 21:56:08 GMT

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


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