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