Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small things #30

Open
Philippe91 opened this issue Jul 23, 2021 · 3 comments
Open

Small things #30

Philippe91 opened this issue Jul 23, 2021 · 3 comments

Comments

@Philippe91
Copy link

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;

@rigtorp
Copy link
Owner

rigtorp commented Jul 26, 2021

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.

@Philippe91
Copy link
Author

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.

@rigtorp
Copy link
Owner

rigtorp commented Jul 26, 2021

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 added a commit that referenced this issue Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants