SPSCQueue icon indicating copy to clipboard operation
SPSCQueue copied to clipboard

Small things

Open Philippe91 opened this issue 3 years ago • 3 comments

First of all, kudos to your cache-aware indices. Though the underlying logic is not obvious (!), it does work and improves performances.

Two small things, not new to version 1.1:

  1. The line: char padding_[kCacheLineSize - sizeof(writeIdxCache_)] has no effect as the global structure has a size that is already quantized, because of alignas(kCacheLineSize) members.

  2. The way you compute kPadding is not optimal, because if sizeof(T) == kCacheLineSize, then kPadding is 1, while 0 would be better.

static constexpr size_t kPadding = (kCacheLineSize - 1) / sizeof(T) + 1;

Philippe91 avatar Jul 23 '21 14:07 Philippe91

1. The line:
   `char padding_[kCacheLineSize - sizeof(writeIdxCache_)]`
   has no effect as the global structure has a size that is already quantized, because of `alignas(kCacheLineSize)` members.

It's required if compiling using C++14 or earlier since alignment is not guaranteed for over-aligned types. https://stackoverflow.com/questions/49373287/gcc-over-aligned-new-support-alignas

2. The way you compute `kPadding `is not optimal, because if sizeof(T) == kCacheLineSize, then `kPadding `is 1, while 0 would be better.

static constexpr size_t kPadding = (kCacheLineSize - 1) / sizeof(T) + 1;

Just because sizeof(T)==kCacheLineSize doesn't mean that alignment of T is kCacheLineSize. You could do something better by taking alignment into account.

rigtorp avatar Jul 26 '21 17:07 rigtorp

The stackoverflow link you mention is about support for aligned-new support, which is not the same thing as padding.

Compilers automatically insert padding at the end of a struct, to ensure that arrays of the struct will be properly aligned. This does not depend on the C++ version. http://eel.is/c++draft/expr.sizeof#2

Just because sizeof(T)==kCacheLineSize doesn't mean that alignment of T is kCacheLineSize. You could do something better by taking alignment into account.

This is right. At the same time, if T is aligned on kCacheLineSize, then its size is commonly sizeof(T) for the reason mention earlier. I say "commonly", because we typically store small objects in concurrent queues. In this case, my original remark stands.

Philippe91 avatar Jul 26 '21 20:07 Philippe91

Compilers automatically insert padding at the end of a struct, to ensure that arrays of the struct will be properly aligned. This does not depend on the C++ version. http://eel.is/c++draft/expr.sizeof#2

I see what you are saying, the padding will always be there. If overaligned new is not supported the padding will still be there. Thanks!

This is right. At the same time, if T is aligned on kCacheLineSize, then its size is commonly sizeof(T) for the reason mention earlier. I say "commonly", because we typically store small objects in concurrent queues. In this case, my original remark stands.

Better to waste some memory and be safe than sorry. Using constexpr and alignof(T) I could make padding 0 if alignof(T) > kCacheLineSize.

rigtorp avatar Jul 26 '21 23:07 rigtorp