Re: Review Request 68001: Fix padding in MpscLinkedQueue.

2018-07-24 Thread Mesos Reviewbot Windows 

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['68001']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1975/mesos-review-68001

Relevant logs:

- 
[libprocess-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1975/mesos-review-68001/logs/libprocess-tests-cmake-stdout.log):

```
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(1520):
 warning C4244: '+=': conversion from '__int64' to 'uint32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(1689):
 warning C4244: '+=': conversion from '__int64' to 'uint32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2273):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2280):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2292):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2301):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2401):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 cl : Command line warning D9002: ignoring unknown option '-fPIC' 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(1415):
 warning C4244: '+=': conversion from '__int64' to 'uint32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(1520):
 warning C4244: '+=': conversion from '__int64' to 'uint32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(1689):
 warning C4244: '+=': conversion from '__int64' to 'uint32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2273):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2280):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2292):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data

Re: Review Request 68001: Fix padding in MpscLinkedQueue.

2018-07-24 Thread Dario Rexin 

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

2018-07-24 Thread Dario Rexin 

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

2018-07-24 Thread James Peach 


> On July 24, 2018, 5:05 p.m., James Peach wrote:
> > 3rdparty/libprocess/src/mpsc_linked_queue.hpp
> > Lines 27 (patched)
> > 
> >
> > Chatted with @bbannier, and he pointed out that we can replace 
> > `__COUNTER__` with something like this:
> > 
> > ```
> > #define MPSC_CACHELINE_PAD(varname) \
> >   uint8_t CAT(_pad, __, varname)[64 - (sizeof(var) % 64)]
> > ```
> 
> Benjamin Bannier wrote:
> I am not sure we still need the macro since the `alignas` will enforce 
> the memory layout we want for `head` and `tail`, and we only really need to 
> add it once to add additional padding after `tail`.
> 
> I'd either suggest getting rid of the macro and just manually adding a 
> padding variable after `tail` (its only required use site), or alternatively 
> move its definition down into the class and `#undef`'ing it after last use. 
> To me the former looks simpler, cleaner and easier to understand.

If we don't add the padding after the head, I expect we will still get the 
clang-tidy warning:

```
consider reordering the fields or adding explicit padding members 
[clang-analyzer-optin.performance.Padding]

```

For consistency, I suggested to @dario that we apply both the alignas and the 
padding for both variables.


- James


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


On July 24, 2018, 2:59 p.m., Dario Rexin wrote:
> 
> ---
> 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
> -
> 
>   3rdparty/libprocess/src/mpsc_linked_queue.hpp 48c95093d 
> 
> 
> Diff: https://reviews.apache.org/r/68001/diff/2/
> 
> 
> Testing
> ---
> 
> make check & benchmarks
> 
> 
> Thanks,
> 
> Dario Rexin
> 
>

Re: Review Request 68001: Fix padding in MpscLinkedQueue.

2018-07-24 Thread Benjamin Bannier 


> On July 24, 2018, 7:05 p.m., James Peach wrote:
> > 3rdparty/libprocess/src/mpsc_linked_queue.hpp
> > Lines 27 (patched)
> > 
> >
> > Chatted with @bbannier, and he pointed out that we can replace 
> > `__COUNTER__` with something like this:
> > 
> > ```
> > #define MPSC_CACHELINE_PAD(varname) \
> >   uint8_t CAT(_pad, __, varname)[64 - (sizeof(var) % 64)]
> > ```

I am not sure we still need the macro since the `alignas` will enforce the 
memory layout we want for `head` and `tail`, and we only really need to add it 
once to add additional padding after `tail`.

I'd either suggest getting rid of the macro and just manually adding a padding 
variable after `tail` (its only required use site), or alternatively move its 
definition down into the class and `#undef`'ing it after last use. To me the 
former looks simpler, cleaner and easier to understand.


- Benjamin


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


On July 24, 2018, 4:59 p.m., Dario Rexin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68001/
> ---
> 
> (Updated July 24, 2018, 4: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
> -
> 
>   3rdparty/libprocess/src/mpsc_linked_queue.hpp 48c95093d 
> 
> 
> Diff: https://reviews.apache.org/r/68001/diff/2/
> 
> 
> Testing
> ---
> 
> make check & benchmarks
> 
> 
> Thanks,
> 
> Dario Rexin
> 
>

Re: Review Request 68001: Fix padding in MpscLinkedQueue.

2018-07-24 Thread Mesos Reviewbot Windows 

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['68001']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1973/mesos-review-68001

Relevant logs:

- 
[libprocess-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1973/mesos-review-68001/logs/libprocess-tests-cmake-stdout.log):

```
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2280):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2292):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2301):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2401):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 cl : Command line warning D9002: ignoring unknown option '-fPIC' 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(1415):
 warning C4244: '+=': conversion from '__int64' to 'uint32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(1520):
 warning C4244: '+=': conversion from '__int64' to 'uint32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(1689):
 warning C4244: '+=': conversion from '__int64' to 'uint32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2273):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2280):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2292):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2301):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2401):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]


   "D:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj" 
(default target) (1) ->
   "D:\DCOS\mesos\3rdparty\libprocess\src\tests\benchmarks.vcxproj" 
(default

Re: Review Request 68001: Fix padding in MpscLinkedQueue.

2018-07-24 Thread Dario Rexin 

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

2018-07-23 Thread James Peach 


> On July 20, 2018, 9:27 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/mpsc_linked_queue.hpp
> > Lines 184-188 (patched)
> > 
> >
> > 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`.
> 
> Dario Rexin wrote:
> 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.

I concur with Dario that it's better to be explicit about the padding. This 
would ensure no false sharing occurs when a `MpscLinkedQueue` instance is 
followed by another variable that has no explicit alignment. We should capture 
this rationale (and a summary of this discussion) in a code comment.

FWIW, the only usage of `MpscLinkedQueue` is:
```C
class EventQueue
{
   MpscLinkedQueue queue;
   std::atomic comissioned = ATOMIC_VAR_INIT(true);
};
```

Experimentally, gcc doesn't add the padding we want in this case:
```C
#include 
#include 
#include 

struct Event
{
alignas(64) std::atomic p1;
alignas(64) std::atomic p2;
std::atomic b;
};

Event e;

int main()
{
printf("p1 -> %zu\n", offsetof(Event, p1));
printf("p1 -> %zu\n", offsetof(Event, p2));
printf("b -> %zu\n", offsetof(Event, b));
}

$ c++ -std=c++14 ./e.cc  && ./a.out
p1 -> 0
p1 -> 64
b -> 72
```


- James


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

2018-07-23 Thread Dario Rexin 


> On July 20, 2018, 9:27 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/mpsc_linked_queue.hpp
> > Lines 184-188 (patched)
> > 
> >
> > 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.

2018-07-23 Thread Benjamin Bannier 


> On July 20, 2018, 11:27 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/mpsc_linked_queue.hpp
> > Lines 184-188 (patched)
> > 
> >
> > 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?

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


- Benjamin


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


On July 20, 2018, 6: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, 6: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.

2018-07-23 Thread Dario Rexin 


> On July 20, 2018, 9:27 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/mpsc_linked_queue.hpp
> > Lines 184-188 (patched)
> > 
> >
> > 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 68001: Fix padding in MpscLinkedQueue.

2018-07-20 Thread Benjamin Bannier 


> On July 20, 2018, 11:27 p.m., Benjamin Bannier wrote:
> > 3rdparty/libprocess/src/mpsc_linked_queue.hpp
> > Lines 184-188 (patched)
> > 
> >
> > 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`).

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


- Benjamin


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


On July 20, 2018, 6: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, 6: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.

2018-07-20 Thread Mesos Reviewbot Windows 

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['68001']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1965/mesos-review-68001

Relevant logs:

- 
[libprocess-tests-cmake-stdout.log](http://dcos-win.westus.cloudapp.azure.com/artifacts/mesos-reviewbot-testing/1965/mesos-review-68001/logs/libprocess-tests-cmake-stdout.log):

```
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2280):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2292):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2301):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2401):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 cl : Command line warning D9002: ignoring unknown option '-fPIC' 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(1415):
 warning C4244: '+=': conversion from '__int64' to 'uint32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(1520):
 warning C4244: '+=': conversion from '__int64' to 'uint32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(1689):
 warning C4244: '+=': conversion from '__int64' to 'uint32_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2273):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2280):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2292):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2301):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]
 
d:\dcos\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2\http_parser.c(2401):
 warning C4244: '=': conversion from '__int64' to 'uint16_t', possible loss of 
data 
[D:\DCOS\mesos\3rdparty\http_parser-2.6.2\src\http_parser-2.6.2-build\http_parser.vcxproj]
 [D:\DCOS\mesos\3rdparty\http_parser-2.6.2.vcxproj]


   "D:\DCOS\mesos\3rdparty\libprocess\src\tests\libprocess-tests.vcxproj" 
(default target) (1) ->
   "D:\DCOS\mesos\3rdparty\libprocess\src\tests\benchmarks.vcxproj" 
(default