mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: Review Request 68001: Fix padding in MpscLinkedQueue.
Date Tue, 24 Jul 2018 18:03:38 GMT


> On July 24, 2018, 5:05 p.m., James Peach wrote:
> > 3rdparty/libprocess/src/mpsc_linked_queue.hpp
> > Lines 27 (patched)
> > <https://reviews.apache.org/r/68001/diff/2/?file=2063433#file2063433line27>
> >
> >     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
> 
>


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