Dedicate cacheline for DB mutex #9637
Conversation
|
@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
1 similar comment
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
ajkr
force-pushed
the
db-mutex-dedicated-cachelines
branch
from
44cb1fe
to
8538f8b
Compare
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
db/db_impl/db_impl.h
Outdated
| // | ||
| // `mutex_` can be a hot lock in some workloads, so it deserves dedicated | ||
| // cachelines. | ||
| ALIGN_AS(CACHE_LINE_SIZE) mutable InstrumentedMutex mutex_; |
There was a problem hiding this comment.
Or can we make the following default_cf_handle_ also aligned on cache line size.
Both the approach in the PR and the above proposal require that the members are in the same access block, e.g. private or public or protected. I hope we can enforce this and not have to worry about (accidental and unlikely) following change:
ALIGN_AS(CACHE_LINE_SIZE) mutable InstrumentedMutex mutex_;
private:
char padding[(CACHE_LINE_SIZE -
(sizeof(InstrumentedMutex) % CACHE_LINE_SIZE)) %
CACHE_LINE_SIZE] ROCKSDB_FIELD_UNUSED;
How about wrap the mutex_ and padding in another struct? This may incur more changes, so I would not strongly advocate it.
There was a problem hiding this comment.
Let me check if it's possible to make a CacheAlignedInstrumentedMutex structure without too much changes.
There was a problem hiding this comment.
Done. C++17 made this much easier. Thanks for the review!
|
@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@ajkr has updated the pull request. You must reimport the pull request before landing. |
|
@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: We found a case of cacheline bouncing due to writers locking/unlocking `mutex_` and readers accessing `block_cache_tracer_`. We discovered it only after the issue was fixed by #9462 shifting the `DBImpl` members such that `mutex_` and `block_cache_tracer_` were naturally placed in separate cachelines in our regression testing setup. This PR forces the cacheline alignment of `mutex_` so we don't accidentally reintroduce the problem. Pull Request resolved: #9637 Reviewed By: riversand963 Differential Revision: D34502233 Pulled By: ajkr fbshipit-source-id: 46aa313b7fe83e80c3de254e332b6fb242434c07
We found a case of cacheline bouncing due to writers locking/unlocking
mutex_and readers accessingblock_cache_tracer_. We discovered it only after the issue was fixed by #9462 shifting theDBImplmembers such thatmutex_andblock_cache_tracer_were naturally placed in separate cachelines in our regression testing setup. This PR forces the cacheline alignment ofmutex_so we don't accidentally reintroduce the problem.