Re: mesos git commit: Added mpsc_linked_queue and use it as the concurrent event queue.
Btw. I use 128 bytes here because of PowerPC. We don’t depend on it to perform well on PPC, but since there has been some work around PPC support I thought I’d make it work for it as well. If you think we should default to x86 cache line size, I’ll change that as well. > On Jul 16, 2018, at 8:39 AM, Dario Rexin wrote: > > Hi Benjamin, > > see comments inline. > >> On Jul 16, 2018, at 5:48 AM, Benjamin Bannier >> wrote: >> >> Hi Dario, >> >> this patch introduced two new clang-tidy warnings. Could we try to get these >> down to zero, even if the code does not look bad? >> >> >> I already created a patch for the unused lambda capture, >> >> https://reviews.apache.org/r/67927/ >> >> While the code does look reasonable, as a somewhat weird exception C++ >> allows referencing some variables without capturing them. >> > > Yes, I was a bit confused by the MSVC error there. I added the explicit > capture because I thought it would be preferable over implicit capture, but > I’m fine with either. Thanks for creating the patch! > >> >> I also looked into the warning on the “excessive padding”. Adding some >> explicit padding seems to make clang-tidy content, but I wasn’t sure whether >> we just wanted to put `head` and `tail` on separate cache lines, or also >> cared about the padding added after `tail`. >> >> private: >> std::atomic*> head; >> >> char padding[128 - sizeof(std::atomic*>)]; >> >> // TODO(drexin): Programatically get the cache line size. >> alignas(128) Node* tail; // FIXME: IMO no need for `alignas` to >> separate `head` and `tail`. >> >> Could you put up a patch for that? You can run the linter yourself; it is >> `support/mesos-tidy.sh`. >> > > That’s interesting. The padding after tail is a good point, we should > definitely add that to prevent false sharing. If we add the padding, is > alignas still necessary? > > Thanks, > Dario > >> >> Cheers, >> >> Benjamin >> >> >>> On Jul 15, 2018, at 7:02 PM, b...@apache.org wrote: >>> >>> Repository: mesos >>> Updated Branches: >>> refs/heads/master a11a6a3d8 -> b1eafc035 >>> >>> >>> Added mpsc_linked_queue and use it as the concurrent event queue. >>> >>> https://reviews.apache.org/r/62515 >>> >>> >>> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo >>> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/b1eafc03 >>> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/b1eafc03 >>> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/b1eafc03 >>> >>> Branch: refs/heads/master >>> Commit: b1eafc035426bc39df4dba81c5c46b8b2d970339 >>> Parents: a11a6a3 >>> Author: Dario Rexin >>> Authored: Sat Jul 7 13:20:22 2018 -0700 >>> Committer: Benjamin Hindman >>> Committed: Sun Jul 15 09:55:28 2018 -0700 >>> >>> -- >>> 3rdparty/libprocess/Makefile.am | 1 + >>> 3rdparty/libprocess/src/event_queue.hpp | 168 ++--- >>> 3rdparty/libprocess/src/mpsc_linked_queue.hpp | 179 +++ >>> 3rdparty/libprocess/src/tests/CMakeLists.txt| 1 + >>> 3rdparty/libprocess/src/tests/benchmarks.cpp| 64 ++- >>> .../src/tests/mpsc_linked_queue_tests.cpp | 104 +++ >>> 6 files changed, 367 insertions(+), 150 deletions(-) >>> -- >>> >>> >>> http://git-wip-us.apache.org/repos/asf/mesos/blob/b1eafc03/3rdparty/libprocess/Makefile.am >>> -- >>> diff --git a/3rdparty/libprocess/Makefile.am >>> b/3rdparty/libprocess/Makefile.am >>> index 2d356aa..631491a 100644 >>> --- a/3rdparty/libprocess/Makefile.am >>> +++ b/3rdparty/libprocess/Makefile.am >>> @@ -307,6 +307,7 @@ libprocess_tests_SOURCES = >>> \ >>> src/tests/loop_tests.cpp\ >>> src/tests/main.cpp \ >>> src/tests/metrics_tests.cpp \ >>> + src/tests/mpsc_linked_queue_tests.cpp\ >>> src/tests/mutex_tests.cpp \ >>> src/tests/owned_tests.cpp \ >>> src/tests/process_tests.cpp \ >>> >>> http://git-wip-us.apache.org/repos/asf/mesos/blob/b1eafc03/3rdparty/libprocess/src/event_queue.hpp >>> -- >>> diff --git a/3rdparty/libprocess/src/event_queue.hpp >>> b/3rdparty/libprocess/src/event_queue.hpp >>> index 21c522d..999d552 100644 >>> --- a/3rdparty/libprocess/src/event_queue.hpp >>> +++ b/3rdparty/libprocess/src/event_queue.hpp >>> @@ -17,10 +17,6 @@ >>> #include >>> #include >>> >>> -#ifdef LOCK_FREE_EVENT_QUEUE >>> -#include >>> -#endif // LOCK_FREE_EVENT_QUEUE >>> - >>> #include >>> #include >>> >>> @@ -28,6 +24,10 @@ >>> #include
Re: mesos git commit: Added mpsc_linked_queue and use it as the concurrent event queue.
Hi Benjamin,
see comments inline.
> On Jul 16, 2018, at 5:48 AM, Benjamin Bannier
> wrote:
>
> Hi Dario,
>
> this patch introduced two new clang-tidy warnings. Could we try to get these
> down to zero, even if the code does not look bad?
>
>
> I already created a patch for the unused lambda capture,
>
>https://reviews.apache.org/r/67927/
>
> While the code does look reasonable, as a somewhat weird exception C++ allows
> referencing some variables without capturing them.
>
Yes, I was a bit confused by the MSVC error there. I added the explicit capture
because I thought it would be preferable over implicit capture, but I’m fine
with either. Thanks for creating the patch!
>
> I also looked into the warning on the “excessive padding”. Adding some
> explicit padding seems to make clang-tidy content, but I wasn’t sure whether
> we just wanted to put `head` and `tail` on separate cache lines, or also
> cared about the padding added after `tail`.
>
> private:
> std::atomic*> head;
>
> char padding[128 - sizeof(std::atomic*>)];
>
> // TODO(drexin): Programatically get the cache line size.
> alignas(128) Node* tail; // FIXME: IMO no need for `alignas` to
> separate `head` and `tail`.
>
> Could you put up a patch for that? You can run the linter yourself; it is
> `support/mesos-tidy.sh`.
>
That’s interesting. The padding after tail is a good point, we should
definitely add that to prevent false sharing. If we add the padding, is alignas
still necessary?
Thanks,
Dario
>
> Cheers,
>
> Benjamin
>
>
>> On Jul 15, 2018, at 7:02 PM, b...@apache.org wrote:
>>
>> Repository: mesos
>> Updated Branches:
>> refs/heads/master a11a6a3d8 -> b1eafc035
>>
>>
>> Added mpsc_linked_queue and use it as the concurrent event queue.
>>
>> https://reviews.apache.org/r/62515
>>
>>
>> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
>> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/b1eafc03
>> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/b1eafc03
>> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/b1eafc03
>>
>> Branch: refs/heads/master
>> Commit: b1eafc035426bc39df4dba81c5c46b8b2d970339
>> Parents: a11a6a3
>> Author: Dario Rexin
>> Authored: Sat Jul 7 13:20:22 2018 -0700
>> Committer: Benjamin Hindman
>> Committed: Sun Jul 15 09:55:28 2018 -0700
>>
>> --
>> 3rdparty/libprocess/Makefile.am | 1 +
>> 3rdparty/libprocess/src/event_queue.hpp | 168 ++---
>> 3rdparty/libprocess/src/mpsc_linked_queue.hpp | 179 +++
>> 3rdparty/libprocess/src/tests/CMakeLists.txt| 1 +
>> 3rdparty/libprocess/src/tests/benchmarks.cpp| 64 ++-
>> .../src/tests/mpsc_linked_queue_tests.cpp | 104 +++
>> 6 files changed, 367 insertions(+), 150 deletions(-)
>> --
>>
>>
>> http://git-wip-us.apache.org/repos/asf/mesos/blob/b1eafc03/3rdparty/libprocess/Makefile.am
>> --
>> diff --git a/3rdparty/libprocess/Makefile.am
>> b/3rdparty/libprocess/Makefile.am
>> index 2d356aa..631491a 100644
>> --- a/3rdparty/libprocess/Makefile.am
>> +++ b/3rdparty/libprocess/Makefile.am
>> @@ -307,6 +307,7 @@ libprocess_tests_SOURCES =
>> \
>> src/tests/loop_tests.cpp\
>> src/tests/main.cpp \
>> src/tests/metrics_tests.cpp \
>> + src/tests/mpsc_linked_queue_tests.cpp \
>> src/tests/mutex_tests.cpp \
>> src/tests/owned_tests.cpp \
>> src/tests/process_tests.cpp \
>>
>> http://git-wip-us.apache.org/repos/asf/mesos/blob/b1eafc03/3rdparty/libprocess/src/event_queue.hpp
>> --
>> diff --git a/3rdparty/libprocess/src/event_queue.hpp
>> b/3rdparty/libprocess/src/event_queue.hpp
>> index 21c522d..999d552 100644
>> --- a/3rdparty/libprocess/src/event_queue.hpp
>> +++ b/3rdparty/libprocess/src/event_queue.hpp
>> @@ -17,10 +17,6 @@
>> #include
>> #include
>>
>> -#ifdef LOCK_FREE_EVENT_QUEUE
>> -#include
>> -#endif // LOCK_FREE_EVENT_QUEUE
>> -
>> #include
>> #include
>>
>> @@ -28,6 +24,10 @@
>> #include
>> #include
>>
>> +#ifdef LOCK_FREE_EVENT_QUEUE
>> +#include "mpsc_linked_queue.hpp"
>> +#endif // LOCK_FREE_EVENT_QUEUE
>> +
>> namespace process {
>>
>> // A _multiple_ producer (MP) _single_ consumer (SC) event queue for a
>> @@ -187,185 +187,55 @@ private:
>> #else // LOCK_FREE_EVENT_QUEUE
>> void enqueue(Event* event)
>> {
>> -Item item = {sequence.fetch_add(1), event};
>>if (comissioned.load()) {
>> - queue.enqueue(std::move(item));
>>
Re: mesos git commit: Added mpsc_linked_queue and use it as the concurrent event queue.
Hi Dario,
this patch introduced two new clang-tidy warnings. Could we try to get these
down to zero, even if the code does not look bad?
I already created a patch for the unused lambda capture,
https://reviews.apache.org/r/67927/
While the code does look reasonable, as a somewhat weird exception C++ allows
referencing some variables without capturing them.
I also looked into the warning on the “excessive padding”. Adding some explicit
padding seems to make clang-tidy content, but I wasn’t sure whether we just
wanted to put `head` and `tail` on separate cache lines, or also cared about
the padding added after `tail`.
private:
std::atomic*> head;
char padding[128 - sizeof(std::atomic*>)];
// TODO(drexin): Programatically get the cache line size.
alignas(128) Node* tail; // FIXME: IMO no need for `alignas` to
separate `head` and `tail`.
Could you put up a patch for that? You can run the linter yourself; it is
`support/mesos-tidy.sh`.
Cheers,
Benjamin
> On Jul 15, 2018, at 7:02 PM, b...@apache.org wrote:
>
> Repository: mesos
> Updated Branches:
> refs/heads/master a11a6a3d8 -> b1eafc035
>
>
> Added mpsc_linked_queue and use it as the concurrent event queue.
>
> https://reviews.apache.org/r/62515
>
>
> Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
> Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/b1eafc03
> Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/b1eafc03
> Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/b1eafc03
>
> Branch: refs/heads/master
> Commit: b1eafc035426bc39df4dba81c5c46b8b2d970339
> Parents: a11a6a3
> Author: Dario Rexin
> Authored: Sat Jul 7 13:20:22 2018 -0700
> Committer: Benjamin Hindman
> Committed: Sun Jul 15 09:55:28 2018 -0700
>
> --
> 3rdparty/libprocess/Makefile.am | 1 +
> 3rdparty/libprocess/src/event_queue.hpp | 168 ++---
> 3rdparty/libprocess/src/mpsc_linked_queue.hpp | 179 +++
> 3rdparty/libprocess/src/tests/CMakeLists.txt| 1 +
> 3rdparty/libprocess/src/tests/benchmarks.cpp| 64 ++-
> .../src/tests/mpsc_linked_queue_tests.cpp | 104 +++
> 6 files changed, 367 insertions(+), 150 deletions(-)
> --
>
>
> http://git-wip-us.apache.org/repos/asf/mesos/blob/b1eafc03/3rdparty/libprocess/Makefile.am
> --
> diff --git a/3rdparty/libprocess/Makefile.am b/3rdparty/libprocess/Makefile.am
> index 2d356aa..631491a 100644
> --- a/3rdparty/libprocess/Makefile.am
> +++ b/3rdparty/libprocess/Makefile.am
> @@ -307,6 +307,7 @@ libprocess_tests_SOURCES =
> \
> src/tests/loop_tests.cpp\
> src/tests/main.cpp \
> src/tests/metrics_tests.cpp \
> + src/tests/mpsc_linked_queue_tests.cpp \
> src/tests/mutex_tests.cpp \
> src/tests/owned_tests.cpp \
> src/tests/process_tests.cpp \
>
> http://git-wip-us.apache.org/repos/asf/mesos/blob/b1eafc03/3rdparty/libprocess/src/event_queue.hpp
> --
> diff --git a/3rdparty/libprocess/src/event_queue.hpp
> b/3rdparty/libprocess/src/event_queue.hpp
> index 21c522d..999d552 100644
> --- a/3rdparty/libprocess/src/event_queue.hpp
> +++ b/3rdparty/libprocess/src/event_queue.hpp
> @@ -17,10 +17,6 @@
> #include
> #include
>
> -#ifdef LOCK_FREE_EVENT_QUEUE
> -#include
> -#endif // LOCK_FREE_EVENT_QUEUE
> -
> #include
> #include
>
> @@ -28,6 +24,10 @@
> #include
> #include
>
> +#ifdef LOCK_FREE_EVENT_QUEUE
> +#include "mpsc_linked_queue.hpp"
> +#endif // LOCK_FREE_EVENT_QUEUE
> +
> namespace process {
>
> // A _multiple_ producer (MP) _single_ consumer (SC) event queue for a
> @@ -187,185 +187,55 @@ private:
> #else // LOCK_FREE_EVENT_QUEUE
> void enqueue(Event* event)
> {
> -Item item = {sequence.fetch_add(1), event};
> if (comissioned.load()) {
> - queue.enqueue(std::move(item));
> + queue.enqueue(event);
> } else {
> - sequence.fetch_sub(1);
> delete event;
> }
> }
>
> Event* dequeue()
> {
> -// NOTE: for performance reasons we don't check `comissioned` here
> -// so it's possible that we'll loop forever if a consumer called
> -// `decomission()` and then subsequently called `dequeue()`.
> -Event* event = nullptr;
> -do {
> - // Given the nature of the concurrent queue implementation it's
> - // possible that we'll need to try to dequeue multiple times
> - // until it returns an event even though we know there is an
> - // event because
