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 61058: Added a lock-free event queue.
Date Wed, 26 Jul 2017 02:08:01 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61058/#review181297
-----------------------------------------------------------




3rdparty/libprocess/configure.ac
Lines 72-75 (original), 72-80 (patched)
<https://reviews.apache.org/r/61058/#comment256823>

    Some TODOs to make the lock free queues the default configuration would be great.
    
    Also, be sure to add this to `docs/configuration.md`.



3rdparty/libprocess/src/event_queue.hpp
Lines 127-136 (patched)
<https://reviews.apache.org/r/61058/#comment256980>

    Hm.. if a thread is checking `empty()` after the sequence is incremented and before its
decremented back in the decomissioned case, it will think incorrectly think it's not empty?
    
    Shouldn't `empty()` check `decomissioned` first, since once set the queue is empty or
being actively emptied by `decomission()`?



3rdparty/libprocess/src/event_queue.hpp
Lines 138-145 (patched)
<https://reviews.apache.org/r/61058/#comment256982>

    Interesting that you took a different strategy here compared to the other queue, might
be worth a comment saying we have to spin due to the nature of `try_dequeue`, and that's ok
since the caller *must* check empty before calling this?



3rdparty/libprocess/src/event_queue.hpp
Lines 147-150 (patched)
<https://reviews.apache.org/r/61058/#comment256986>

    There seems to be a requirement that only the single consumer is the one that calls decomission
and that once the consumer calls decomission, no more empty->dequeue looping will occur.
Otherwise, the consumer can loop:
    
    (1) decomission, but not drained yet
    (2) consumer checks empty, returns true since not yet drained by decomission
    (3) consumer then calls dequeue
    (4) decomission completes the drain
    (5) consumer loops in dequeue
    
    Or:
    
    (1) decomission, drained
    (2) producer calls enqueue, sequence is incremented
    (3) consumer checks empty, returns true since sequence was incremented
    (5 optional) producer then finishes call to enqueue by decrementing sequence
    (4) consumer calls dequeue, loops



3rdparty/libprocess/src/event_queue.hpp
Lines 149 (patched)
<https://reviews.apache.org/r/61058/#comment256985>

    The minus 1 here and plus 1s below seem odd, can we remove the off by one logic if sequence
starts at 0 instead of 1? It looks like it to me.



3rdparty/libprocess/src/event_queue.hpp
Lines 170-182 (patched)
<https://reviews.apache.org/r/61058/#comment256987>

    Shouldn't this trigger a drain into items before we count? As is done with the array operator
below.
    
    Also, is the caller of count necessarily the consumer? It seems also that count is only
safe to call by a single consumer, which I guess requires that count be called within the
context of its owning Process. I seem to have dejavu here, did you already document that requirement
somewhere in an earlier change?



3rdparty/libprocess/src/event_queue.hpp
Lines 184-198 (patched)
<https://reviews.apache.org/r/61058/#comment256990>

    Array seems to require that it be called by the single consumer? Otherwise there is concurrent
access to items.
    
    It would be great to clarify this, or maybe offer EventQueue::Consumer and EventQueue::Producer
to make the consumer interface part of the type system.



3rdparty/libprocess/src/event_queue.hpp
Lines 186-187 (patched)
<https://reviews.apache.org/r/61058/#comment256988>

    Why 256 instead of unbounded dequeue?



3rdparty/libprocess/src/event_queue.hpp
Lines 193 (patched)
<https://reviews.apache.org/r/61058/#comment256989>

    std::move?



3rdparty/libprocess/src/event_queue.hpp
Lines 209-211 (patched)
<https://reviews.apache.org/r/61058/#comment256991>

    Shouldn't an item smaller than the next entry we expect be something we CHECK against
rather than silently drop?



3rdparty/libprocess/src/event_queue.hpp
Lines 209-229 (patched)
<https://reviews.apache.org/r/61058/#comment256994>

    It took me a long time to understand this code, I suspect it will be easier to read if
we were to store 'next' instead of 'last', since the intent here is to find the next entry
and the off by ones go away AFAICT.



3rdparty/libprocess/src/event_queue.hpp
Lines 220-229 (patched)
<https://reviews.apache.org/r/61058/#comment256995>

    I assume the linear search here is fine because the item is likely near the front? Might
want to clarify that for the reader?



3rdparty/libprocess/src/event_queue.hpp
Lines 239 (patched)
<https://reviews.apache.org/r/61058/#comment256976>

    



3rdparty/libprocess/src/event_queue.hpp
Lines 239-240 (patched)
<https://reviews.apache.org/r/61058/#comment256978>

    Hm.. what do these represent and why do we need them? (I'm about to find out from reading
the code but would be nice to explain with a comment here).



3rdparty/libprocess/src/event_queue.hpp
Lines 240 (patched)
<https://reviews.apache.org/r/61058/#comment256981>

    last is not atomic because there is a single consumer and it's only modified by the consumer?
This seems to imply that only the single consumer is allowed to call `empty()`? Seems worth
clarifying, since I would expect others to be able to call empty



3rdparty/libprocess/src/event_queue.hpp
Lines 241 (patched)
<https://reviews.apache.org/r/61058/#comment256979>

    Hm.. seeing a deque was pretty surprising, can you explain here what the implementation
approach is in general with sequence, last, and items (in addition to just the concurrent
queue)?


- Benjamin Mahler


On July 22, 2017, 1:16 a.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61058/
> -----------------------------------------------------------
> 
> (Updated July 22, 2017, 1:16 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7798
>     https://issues.apache.org/jira/browse/MESOS-7798
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added a lock-free event queue.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/configure.ac cb2cf4f32be5cbdf9df1e32f9aaf2bbba0a5ae03 
>   3rdparty/libprocess/src/event_queue.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61058/diff/2/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>


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