-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
SSD cache-dictionary #8624
SSD cache-dictionary #8624
Conversation
I think it is necessary to add performance tests to benchmark it in comparison with DirectDictionary and CacheDictionary. Or just CacheDictionary in memory with SSDCacheDictionary. |
src/Dictionaries/BucketCache.h
Outdated
|
||
namespace | ||
{ | ||
inline size_t nearestPowTwo(size_t x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to use roundUpToPowerOfTwoOrZero
from our codebase
|
||
size_t getPosition(const size_t bucket) const | ||
{ | ||
const size_t idx = (bucket >> 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need it here? bucket it already a number from [0, buckets - 1]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To determine a position in the bucket we need to read 4 bits from the array with positions, but we can address only bytes, so we need to divide bucket number by 2.
}; | ||
|
||
if (!write_buffer) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Braces are redundant according to our codestyle
|
||
const size_t start_block = current_file_block_id % max_size; | ||
const size_t finish_block = start_block + write_buffer_size; | ||
for (const auto& key : keys) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style: for (const auto & key : keys)
{ | ||
// add partitions to queue | ||
while (partitions.size() > max_partitions_count) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style
AIOContext aio_context(1); | ||
|
||
while (io_submit(aio_context.ctx, 1, &request_ptr) != 1) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style
} | ||
|
||
while (io_getevents(aio_context.ctx, 1, 1, &event, nullptr) != 1) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style
|
||
TemporalComplexKeysPool tmp_keys_pool; | ||
storage.update( | ||
source_ptr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it true, that we can use many sources and not only one?
void SSDCachePartition::remove() | ||
{ | ||
std::unique_lock lock(rw_lock); | ||
std::filesystem::remove(std::filesystem::path(path + BIN_FILE_EXT)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error handling?
std::vector<Key> required_ids(not_found_ids.size()); | ||
std::transform(std::begin(not_found_ids), std::end(not_found_ids), std::begin(required_ids), [](const auto & pair) { return pair.first; }); | ||
|
||
storage.update( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe better to wrap it in a separate function, which adds source_ptr and lifetime and call this function?
BTW - it would be really interesting to see some perf number, and/or comparison with aerospike (#5629) |
@filimonov They are published in https://presentations.clickhouse.tech/hse_2020/3rd/SSD_Dictionary_full.pdf |
Just for the reference - aerospike gives up to ~1Mln req/sec |
@filimonov This number does not mean anything. |
Merging #8624 (ssd-cache)
Merged in #11947 |
I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Cache-dictionaries layout for storing data on SSD.
TODO: