Re: Review Request 68149: Replace exchange in MpscLinkedQueue::dequeue with load/store.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68149/ --- (Updated Aug. 5, 2018, 6:02 p.m.) Review request for James Peach. Repository: mesos Description --- There's only a single consumer, so we don't need to exchange in dequeue. Testing shows that a load/store pair is much more efficient, as it needs fewer CPU cycles. Especially in the empty queue case, where the store does not have to be executed at all. Diffs (updated) - 3rdparty/libprocess/src/mpsc_linked_queue.hpp fc55d4389 3rdparty/libprocess/src/tests/benchmarks.cpp 0b94aaf4e Diff: https://reviews.apache.org/r/68149/diff/3/ Changes: https://reviews.apache.org/r/68149/diff/2-3/ Testing --- Make check and benchmarks. Benchmark results: Exchange (before): [===] Running 3 tests from 1 test case. [---] Global test environment set-up. [---] 3 tests from ProcessTest [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueue Estimated producer throughput (7 threads): 38741698.929628 op/s Estimated consumer throughput: 15783395.811778 op/s Estimated total throughput: 54525094.741406 op/s [OK ] ProcessTest.Process_BENCHMARK_MpscLinkedQueue (4435 ms) [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueEmpty Estimated consumer throughput: 182175973.774530 op/s [OK ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueEmpty (5489 ms) [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueNonContendedRead Estimated producer throughput (7 threads): 41807291.688699 op/s Estimated consumer throughput: 21925675.985841 op/s Estimated total throughput: 63732967.674541 op/s Load/Store (after): [===] Running 3 tests from 1 test case. [---] Global test environment set-up. [---] 3 tests from ProcessTest [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueue Estimated producer throughput (7 threads): 39972854.982381 op/s Estimated consumer throughput: 18090177.075559 op/s Estimated total throughput: 58063032.057939 op/s [OK ] ProcessTest.Process_BENCHMARK_MpscLinkedQueue (3870 ms) [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueEmpty Estimated consumer throughput: 1457824631.198832 op/s [OK ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueEmpty (686 ms) [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueNonContendedRead Estimated producer throughput (7 threads): 48031578.693516 op/s Estimated consumer throughput: 25242881.515617 op/s Estimated total throughput: 73274460.209133 op/s File Attachments mpsc-load-store.patch https://reviews.apache.org/media/uploaded/files/2018/08/02/b8bd71a1-a1c7-47b1-beae-dc9f522ae192__mpsc-load-store.patch Thanks, Dario Rexin
Re: Review Request 68149: Replace exchange in MpscLinkedQueue::dequeue with load/store.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68149/ --- (Updated Aug. 2, 2018, 9:44 p.m.) Review request for James Peach. Repository: mesos Description --- There's only a single consumer, so we don't need to exchange in dequeue. Testing shows that a load/store pair is much more efficient, as it needs fewer CPU cycles. Especially in the empty queue case, where the store does not have to be executed at all. Diffs (updated) - 3rdparty/libprocess/src/mpsc_linked_queue.hpp fc55d4389 3rdparty/libprocess/src/tests/benchmarks.cpp 0b94aaf4e Diff: https://reviews.apache.org/r/68149/diff/2/ Changes: https://reviews.apache.org/r/68149/diff/1-2/ Testing --- Make check and benchmarks. Benchmark results: Exchange (before): [===] Running 3 tests from 1 test case. [---] Global test environment set-up. [---] 3 tests from ProcessTest [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueue Estimated producer throughput (7 threads): 38741698.929628 op/s Estimated consumer throughput: 15783395.811778 op/s Estimated total throughput: 54525094.741406 op/s [OK ] ProcessTest.Process_BENCHMARK_MpscLinkedQueue (4435 ms) [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueEmpty Estimated consumer throughput: 182175973.774530 op/s [OK ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueEmpty (5489 ms) [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueNonContendedRead Estimated producer throughput (7 threads): 41807291.688699 op/s Estimated consumer throughput: 21925675.985841 op/s Estimated total throughput: 63732967.674541 op/s Load/Store (after): [===] Running 3 tests from 1 test case. [---] Global test environment set-up. [---] 3 tests from ProcessTest [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueue Estimated producer throughput (7 threads): 39972854.982381 op/s Estimated consumer throughput: 18090177.075559 op/s Estimated total throughput: 58063032.057939 op/s [OK ] ProcessTest.Process_BENCHMARK_MpscLinkedQueue (3870 ms) [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueEmpty Estimated consumer throughput: 1457824631.198832 op/s [OK ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueEmpty (686 ms) [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueNonContendedRead Estimated producer throughput (7 threads): 48031578.693516 op/s Estimated consumer throughput: 25242881.515617 op/s Estimated total throughput: 73274460.209133 op/s File Attachments mpsc-load-store.patch https://reviews.apache.org/media/uploaded/files/2018/08/02/b8bd71a1-a1c7-47b1-beae-dc9f522ae192__mpsc-load-store.patch Thanks, Dario Rexin
Re: Review Request 68149: Replace exchange in MpscLinkedQueue::dequeue with load/store.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68149/ --- (Updated Aug. 2, 2018, 9:43 p.m.) Review request for James Peach. Repository: mesos Description --- There's only a single consumer, so we don't need to exchange in dequeue. Testing shows that a load/store pair is much more efficient, as it needs fewer CPU cycles. Especially in the empty queue case, where the store does not have to be executed at all. Diffs - 3rdparty/libprocess/src/mpsc_linked_queue.hpp fc55d4389 3rdparty/libprocess/src/tests/benchmarks.cpp 0b94aaf4e Diff: https://reviews.apache.org/r/68149/diff/1/ Testing --- Make check and benchmarks. Benchmark results: Exchange (before): [===] Running 3 tests from 1 test case. [---] Global test environment set-up. [---] 3 tests from ProcessTest [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueue Estimated producer throughput (7 threads): 38741698.929628 op/s Estimated consumer throughput: 15783395.811778 op/s Estimated total throughput: 54525094.741406 op/s [OK ] ProcessTest.Process_BENCHMARK_MpscLinkedQueue (4435 ms) [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueEmpty Estimated consumer throughput: 182175973.774530 op/s [OK ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueEmpty (5489 ms) [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueNonContendedRead Estimated producer throughput (7 threads): 41807291.688699 op/s Estimated consumer throughput: 21925675.985841 op/s Estimated total throughput: 63732967.674541 op/s Load/Store (after): [===] Running 3 tests from 1 test case. [---] Global test environment set-up. [---] 3 tests from ProcessTest [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueue Estimated producer throughput (7 threads): 39972854.982381 op/s Estimated consumer throughput: 18090177.075559 op/s Estimated total throughput: 58063032.057939 op/s [OK ] ProcessTest.Process_BENCHMARK_MpscLinkedQueue (3870 ms) [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueEmpty Estimated consumer throughput: 1457824631.198832 op/s [OK ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueEmpty (686 ms) [ RUN ] ProcessTest.Process_BENCHMARK_MpscLinkedQueueNonContendedRead Estimated producer throughput (7 threads): 48031578.693516 op/s Estimated consumer throughput: 25242881.515617 op/s Estimated total throughput: 73274460.209133 op/s File Attachments (updated) mpsc-load-store.patch https://reviews.apache.org/media/uploaded/files/2018/08/02/b8bd71a1-a1c7-47b1-beae-dc9f522ae192__mpsc-load-store.patch Thanks, Dario Rexin
Re: Review Request 68039: Padded using the type name in `MpscLinkedQueue`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68039/#review206461 --- Ship it! Ship It! - Dario Rexin On July 25, 2018, 2:42 p.m., James Peach wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68039/ > --- > > (Updated July 25, 2018, 2:42 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Benjamin Mahler, and Dario > Rexin. > > > Repository: mesos > > > Description > --- > > The Windows build doesn't accept the name of the `tail` member > variable when defining the padding, so use the type name instead. > > > Diffs > - > > 3rdparty/libprocess/src/mpsc_linked_queue.hpp > 0036eb8d6c0a8dc6f78dc552c8f5500305985172 > > > Diff: https://reviews.apache.org/r/68039/diff/1/ > > > Testing > --- > > None (waiting for reviewbot) > > > Thanks, > > James Peach > >
Re: Review Request 68001: Fix padding in MpscLinkedQueue.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68001/ --- (Updated July 24, 2018, 6:06 p.m.) Review request for Benjamin Bannier and James Peach. Repository: mesos Description --- This patch aligns the head of MpscLinkedQueue to a new cache line and adds padding between head and tail to avoid false sharing between to two and after tail to avoid false sharing with other objects that could otherwise end up on the same cache line. Diffs (updated) - 3rdparty/libprocess/src/mpsc_linked_queue.hpp 48c95093d Diff: https://reviews.apache.org/r/68001/diff/4/ Changes: https://reviews.apache.org/r/68001/diff/3-4/ Testing --- make check & benchmarks Thanks, Dario Rexin
Re: Review Request 68001: Fix padding in MpscLinkedQueue.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68001/ --- (Updated July 24, 2018, 6:05 p.m.) Review request for Benjamin Bannier and James Peach. Repository: mesos Description --- This patch aligns the head of MpscLinkedQueue to a new cache line and adds padding between head and tail to avoid false sharing between to two and after tail to avoid false sharing with other objects that could otherwise end up on the same cache line. Diffs (updated) - 3rdparty/libprocess/src/mpsc_linked_queue.hpp 48c95093d Diff: https://reviews.apache.org/r/68001/diff/3/ Changes: https://reviews.apache.org/r/68001/diff/2-3/ Testing --- make check & benchmarks Thanks, Dario Rexin
Re: Review Request 68001: Fix padding in MpscLinkedQueue.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68001/ --- (Updated July 24, 2018, 2:59 p.m.) Review request for Benjamin Bannier and James Peach. Repository: mesos Description --- This patch aligns the head of MpscLinkedQueue to a new cache line and adds padding between head and tail to avoid false sharing between to two and after tail to avoid false sharing with other objects that could otherwise end up on the same cache line. Diffs (updated) - 3rdparty/libprocess/src/mpsc_linked_queue.hpp 48c95093d Diff: https://reviews.apache.org/r/68001/diff/2/ Changes: https://reviews.apache.org/r/68001/diff/1-2/ Testing --- make check & benchmarks Thanks, Dario Rexin
Re: Review Request 68001: Fix padding in MpscLinkedQueue.
> On July 20, 2018, 9:27 p.m., Benjamin Bannier wrote: > > 3rdparty/libprocess/src/mpsc_linked_queue.hpp > > Lines 184-188 (patched) > > <https://reviews.apache.org/r/68001/diff/1/?file=2061817#file2061817line186> > > > > Wouldn't it be possible to avoid all the manual padding by aligning > > both `head` and `tail` on their own cache lines? > > > > // We align `head` to 64 bytes (x86 cache line size) to guarantee > > it to > > // be put on a new cache line. This is to prevent false sharing with > > // other objects that could otherwise end up on the same cache line > > as > > // this queue. We also align `tail` to avoid false sharing of `head` > > // with `tail` and to avoid false sharing with other objects. > > > > // We assume a x86_64 cache line size of 64 bytes. > > // > > // TODO(drexin): Programatically get the cache line size. > > #define CACHE_LINE 64 > > > > alignas(CACHE_LINE) std::atomic*> head; > > alignas(CACHE_LINE) Node* tail; > > > > #undef CACHE_LINE > > > > Wouldn't this satisfy the guarantees you list in your comment? > > > > It would also allows us to avoid the macro which has a number of issues > > (`__COUNTER__` is a GNU extension, missing check of `sizeof(var) < 64`). > > Benjamin Bannier wrote: > I should have used a real variable for `CACHE_LINE` above to make this > less nasty, e.g., > > static constexpr std::size_t CACHE_LINE = 64; // Requires e.g., > `#include `. > > Dario Rexin wrote: > It would probably work for the padding between head and tail, but what > about the padding after tail? Should we `alignas(64) char tailPad;` or > somethign like that? > > Benjamin Bannier wrote: > If we align both `head` and `tail` on separate cache lines, I believe we > cannot have one queue's `tail` share a cache line with e.g., another queue's > `head` > > Do need to worry about `tail` sharing a cache line with arbitrary other > data? If not it would seem we wouldn't need additional padding after `tail`. In general we should worry about other data. Anything that can cause cache line invalidation would be problematic. I don't want to make assumptions about the likelyhood of that happening. I think it's better to be on the safe side. - Dario --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68001/#review206289 --- On July 20, 2018, 4:51 p.m., Dario Rexin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68001/ > --- > > (Updated July 20, 2018, 4:51 p.m.) > > > Review request for Benjamin Bannier. > > > Repository: mesos > > > Description > --- > > This patch aligns the head of MpscLinkedQueue to a new cache line > and adds padding between head and tail to avoid false sharing > between to two and after tail to avoid false sharing with other > objects that could otherwise end up on the same cache line. > > > Diffs > - > > 3rdparty/libprocess/src/mpsc_linked_queue.hpp 48c95093d > > > Diff: https://reviews.apache.org/r/68001/diff/1/ > > > Testing > --- > > make check & benchmarks > > > Thanks, > > Dario Rexin > >
Re: Review Request 68001: Fix padding in MpscLinkedQueue.
> On July 20, 2018, 9:27 p.m., Benjamin Bannier wrote: > > 3rdparty/libprocess/src/mpsc_linked_queue.hpp > > Lines 184-188 (patched) > > <https://reviews.apache.org/r/68001/diff/1/?file=2061817#file2061817line186> > > > > Wouldn't it be possible to avoid all the manual padding by aligning > > both `head` and `tail` on their own cache lines? > > > > // We align `head` to 64 bytes (x86 cache line size) to guarantee > > it to > > // be put on a new cache line. This is to prevent false sharing with > > // other objects that could otherwise end up on the same cache line > > as > > // this queue. We also align `tail` to avoid false sharing of `head` > > // with `tail` and to avoid false sharing with other objects. > > > > // We assume a x86_64 cache line size of 64 bytes. > > // > > // TODO(drexin): Programatically get the cache line size. > > #define CACHE_LINE 64 > > > > alignas(CACHE_LINE) std::atomic*> head; > > alignas(CACHE_LINE) Node* tail; > > > > #undef CACHE_LINE > > > > Wouldn't this satisfy the guarantees you list in your comment? > > > > It would also allows us to avoid the macro which has a number of issues > > (`__COUNTER__` is a GNU extension, missing check of `sizeof(var) < 64`). > > Benjamin Bannier wrote: > I should have used a real variable for `CACHE_LINE` above to make this > less nasty, e.g., > > static constexpr std::size_t CACHE_LINE = 64; // Requires e.g., > `#include `. It would probably work for the padding between head and tail, but what about the padding after tail? Should we `alignas(64) char tailPad;` or somethign like that? - Dario --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68001/#review206289 --- On July 20, 2018, 4:51 p.m., Dario Rexin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68001/ > --- > > (Updated July 20, 2018, 4:51 p.m.) > > > Review request for Benjamin Bannier. > > > Repository: mesos > > > Description > --- > > This patch aligns the head of MpscLinkedQueue to a new cache line > and adds padding between head and tail to avoid false sharing > between to two and after tail to avoid false sharing with other > objects that could otherwise end up on the same cache line. > > > Diffs > - > > 3rdparty/libprocess/src/mpsc_linked_queue.hpp 48c95093d > > > Diff: https://reviews.apache.org/r/68001/diff/1/ > > > Testing > --- > > make check & benchmarks > > > Thanks, > > Dario Rexin > >
Re: Review Request 67921: Bug fix for semaphore decomission "deadlock".
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67921/#review206111 --- Ship it! Ship It! - Dario Rexin On July 15, 2018, 5:09 p.m., Benjamin Hindman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67921/ > --- > > (Updated July 15, 2018, 5:09 p.m.) > > > Review request for mesos and Benjamin Mahler. > > > Bugs: MESOS-8239 > https://issues.apache.org/jira/browse/MESOS-8239 > > > Repository: mesos > > > Description > --- > > Fixes MESOS-8239. > > When using the DecomissionableLastInFirstOutFixedSizeSemaphore it's > possible that waiting threads may never be properly signaled. This bug > fix makes sure that every waiting thread gets a signal after a > decomission. > > > Diffs > - > > 3rdparty/libprocess/src/semaphore.hpp > 50501b9797894ad274eb73f74b3eed00cd719114 > > > Diff: https://reviews.apache.org/r/67921/diff/1/ > > > Testing > --- > > make check > > > Thanks, > > Benjamin Hindman > >
Re: Review Request 62515: Added mpsc_linked_queue and use it as the concurrent event queue.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62515/ --- (Updated June 27, 2018, 7:55 p.m.) Review request for Benjamin Hindman. Repository: mesos Description --- Linked queue Diffs (updated) - 3rdparty/libprocess/Makefile.am de69d04fb 3rdparty/libprocess/src/event_queue.hpp 21c522dcb 3rdparty/libprocess/src/mpsc_linked_queue.hpp PRE-CREATION 3rdparty/libprocess/src/tests/CMakeLists.txt 400b773d9 3rdparty/libprocess/src/tests/benchmarks.cpp 604122a2f 3rdparty/libprocess/src/tests/mpsc_linked_queue_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/62515/diff/7/ Changes: https://reviews.apache.org/r/62515/diff/6-7/ Testing --- File Attachments queue.patch https://reviews.apache.org/media/uploaded/files/2017/09/25/6743874f-97a8-459c-b9e9-e0b615ff372f__queue.patch queue.patch https://reviews.apache.org/media/uploaded/files/2018/02/08/33de7354-3809-402d-806d-e8d9086856f7__queue.patch queue.patch https://reviews.apache.org/media/uploaded/files/2018/06/15/b010f4cb-6492-4192-aa3b-bcbace547274__queue.patch Thanks, Dario Rexin
Re: Review Request 62515: Added mpsc_linked_queue and use it as the concurrent event queue.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62515/ --- (Updated June 27, 2018, 5:18 p.m.) Review request for Benjamin Hindman. Repository: mesos Description --- Linked queue Diffs (updated) - 3rdparty/libprocess/Makefile.am de69d04fb 3rdparty/libprocess/src/event_queue.hpp 21c522dcb 3rdparty/libprocess/src/mpsc_linked_queue.hpp PRE-CREATION 3rdparty/libprocess/src/tests/CMakeLists.txt 400b773d9 3rdparty/libprocess/src/tests/benchmarks.cpp 604122a2f 3rdparty/libprocess/src/tests/mpsc_linked_queue_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/62515/diff/6/ Changes: https://reviews.apache.org/r/62515/diff/5-6/ Testing --- File Attachments queue.patch https://reviews.apache.org/media/uploaded/files/2017/09/25/6743874f-97a8-459c-b9e9-e0b615ff372f__queue.patch queue.patch https://reviews.apache.org/media/uploaded/files/2018/02/08/33de7354-3809-402d-806d-e8d9086856f7__queue.patch queue.patch https://reviews.apache.org/media/uploaded/files/2018/06/15/b010f4cb-6492-4192-aa3b-bcbace547274__queue.patch Thanks, Dario Rexin
Re: Review Request 61058: Added a lock-free event queue.
> On July 31, 2017, 9:05 p.m., Dario Rexin wrote: > > 3rdparty/libprocess/src/event_queue.hpp > > Lines 328 (patched) > > <https://reviews.apache.org/r/61058/diff/3/?file=1785564#file1785564line331> > > > > I don't think this queue matches the requirements for an actor system. > > This queue uses sub-queues as a performance optimization and the ordering > > between the different queues is not guaranteed. The assumption here is, > > that causal ordering only needs to be maintained per thread. In an actor > > system however, there is no guarantee on which thread an actor will run. > > Think of following scenario: > > > > Actor A receives a burst of messages and takes some time to process > > them. > > > > Actor B receives a message gets dispatched to Thread 1 and sends a > > message to Actor A. The queue of Actor B is now empty, so it donates the > > thread back to the pool. > > > > Actor A is still busy processing the messages it received earlier and > > did not process the message that Actor B sent. > > > > Actor B receives another message and gets dispatched to Thread 2. It > > now sends another message to Actor A. > > > > Now Actor A received 2 messages from Actor B from 2 different threads. > > The moodycamel queue does not guarantee the order of those messages. This > > violates the arrival ordering defined in the actor model and makes it > > impossible to reason about message ordering between two actors. > > Benjamin Hindman wrote: > For posterity, to account for this "deficiency" in the concurrent queue > we've forced the _single_ consumer semantics on the queue and let the > consumer reorder any events when the read them out. See the lock-free > implementation of `EventQueue::try_dequeue` for more details. > > Dario Rexin wrote: > Clarified with benh and benm. There's additional code that restores > ordering. -.- I forgot to click publish - Dario --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61058/#review181840 --- On July 29, 2017, 8:52 p.m., Benjamin Hindman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61058/ > --- > > (Updated July 29, 2017, 8:52 p.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 > 3rdparty/libprocess/src/process.cpp > b268cdad776a3ca2a87cbe60eb098bde2a70667c > > > Diff: https://reviews.apache.org/r/61058/diff/3/ > > > Testing > --- > > make check > > > Thanks, > > Benjamin Hindman > >
Re: Review Request 61058: Added a lock-free event queue.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61058/#review181840 --- 3rdparty/libprocess/src/event_queue.hpp Lines 328 (patched) <https://reviews.apache.org/r/61058/#comment257610> I don't think this queue matches the requirements for an actor system. This queue uses sub-queues as a performance optimization and the ordering between the different queues is not guaranteed. The assumption here is, that causal ordering only needs to be maintained per thread. In an actor system however, there is no guarantee on which thread an actor will run. Think of following scenario: Actor A receives a burst of messages and takes some time to process them. Actor B receives a message gets dispatched to Thread 1 and sends a message to Actor A. The queue of Actor B is now empty, so it donates the thread back to the pool. Actor A is still busy processing the messages it received earlier and did not process the message that Actor B sent. Actor B receives another message and gets dispatched to Thread 2. It now sends another message to Actor A. Now Actor A received 2 messages from Actor B from 2 different threads. The moodycamel queue does not guarantee the order of those messages. This violates the arrival ordering defined in the actor model and makes it impossible to reason about message ordering between two actors. - Dario Rexin On July 29, 2017, 8:52 p.m., Benjamin Hindman wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61058/ > --- > > (Updated July 29, 2017, 8:52 p.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 > 3rdparty/libprocess/src/process.cpp > b268cdad776a3ca2a87cbe60eb098bde2a70667c > > > Diff: https://reviews.apache.org/r/61058/diff/3/ > > > Testing > --- > > make check > > > Thanks, > > Benjamin Hindman > >
Re: Review Request 47804: Fixed a memory leak in SchedulerProcess.decline.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47804/ --- (Updated May 25, 2016, 4:09 a.m.) Review request for mesos and Vinod Kone. Changes --- Fixed "Bugs", "People" and "Summary". -- @vinodkone Summary (updated) - Fixed a memory leak in SchedulerProcess.decline. Bugs: MESOS-5449 https://issues.apache.org/jira/browse/MESOS-5449 Repository: mesos Description --- MesosScheduler.declineOffers has been changed ~6 months ago to send a Decline message instead of calling acceptOffers with an empty list of task infos. The changed version of declineOffer however did not remove the offerId from the savedOffers map, causing a memory leak. Diffs - src/sched/sched.cpp 9e55885 Diff: https://reviews.apache.org/r/47804/diff/ Testing --- make check Thanks, Dario Rexin
Re: Review Request 47804: Fixed a memory leak in SchedulerProcess.decline
> On May 25, 2016, 1:35 a.m., Vinod Kone wrote:
> > src/sched/sched.cpp, line 1353
> > <https://reviews.apache.org/r/47804/diff/1/?file=1392955#file1392955line1353>
> >
> > Can you log the warning like we do in acceptOffers()?
> >
> > ```
> > if (!savedOffers.contains(offerId)) {
> > LOG(WARNING) << "Attempting to decline an unknown offer << offerId;
> > }
> >
> > savedOffers.erase(offerId);
> >
> > ```
Thanks for the feedback. I updated the patch accordingly.
- Dario
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47804/#review134676
---
On May 25, 2016, 3:45 a.m., Dario Rexin wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47804/
> ---
>
> (Updated May 25, 2016, 3:45 a.m.)
>
>
> Review request for mesos.
>
>
> Bugs: https://issues.apache.org/jira/browse/MESOS-5449
>
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-5449
>
>
> Repository: mesos
>
>
> Description
> ---
>
> MesosScheduler.declineOffers has been changed ~6 months ago to send a
> Decline message instead of calling acceptOffers with an empty list of
> task infos. The changed version of declineOffer however did not remove
> the offerId from the savedOffers map, causing a memory leak.
>
>
> Diffs
> -
>
> src/sched/sched.cpp 9e55885
>
> Diff: https://reviews.apache.org/r/47804/diff/
>
>
> Testing
> ---
>
> make check
>
>
> Thanks,
>
> Dario Rexin
>
>
Re: Review Request 47804: Fixed a memory leak in SchedulerProcess.decline
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47804/ --- (Updated May 25, 2016, 3:45 a.m.) Review request for mesos. Bugs: https://issues.apache.org/jira/browse/MESOS-5449 https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-5449 Repository: mesos Description (updated) --- MesosScheduler.declineOffers has been changed ~6 months ago to send a Decline message instead of calling acceptOffers with an empty list of task infos. The changed version of declineOffer however did not remove the offerId from the savedOffers map, causing a memory leak. Diffs (updated) - src/sched/sched.cpp 9e55885 Diff: https://reviews.apache.org/r/47804/diff/ Testing --- make check Thanks, Dario Rexin
Review Request 47804: Fixed a memory leak in SchedulerProcess.decline
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47804/ --- Review request for mesos. Bugs: https://issues.apache.org/jira/browse/MESOS-5449 https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-5449 Repository: mesos Description --- MesosScheduler.declineOffers has been changed ~6 months ago to send a Decline message instead of calling acceptOffers with an empty list of task infos. The changed version of declineOffer however did not remove the offerId from the savedOffers map, causing a memory leak. Diffs - src/sched/sched.cpp 9e55885 Diff: https://reviews.apache.org/r/47804/diff/ Testing --- make check Thanks, Dario Rexin
Re: Review Request 47260: Added 'ReviveAndSuppress' test for the allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47260/#review133408 --- Ship it! Ship It! - Dario Rexin On May 12, 2016, 4:01 p.m., Jiang Yan Xu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47260/ > --- > > (Updated May 12, 2016, 4:01 p.m.) > > > Review request for mesos and Dario Rexin. > > > Bugs: MESOS-5279 > https://issues.apache.org/jira/browse/MESOS-5279 > > > Repository: mesos > > > Description > --- > > Added 'ReviveAndSuppress' test for the allocator. > > > Diffs > - > > src/tests/hierarchical_allocator_tests.cpp > e6101fdffe5e340619d79d821a2f5f891cb2dec7 > > Diff: https://reviews.apache.org/r/47260/diff/ > > > Testing > --- > > make check. > > > Thanks, > > Jiang Yan Xu > >
