-
Notifications
You must be signed in to change notification settings - Fork 968
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
chore(tiering): Faster small bins serialization #2 #3396
base: main
Are you sure you want to change the base?
Conversation
4b10617
to
0b4a673
Compare
Signed-off-by: Vladislav Oleshko <[email protected]>
// If tiering is enabled, try saving the received page on disk | ||
// Fall back to memory in case of errors | ||
if (EngineShard::tlocal() && EngineShard::tlocal()->tiered_storage()) { | ||
auto& storage = EngineShard::tlocal()->tiered_storage()->BorrowStorage(); | ||
|
||
util::fb2::Done done; | ||
std::error_code ec; | ||
auto cb = [this, offset, &ec, &done](io::Result<tiering::DiskSegment> res) { | ||
if (res.has_value()) | ||
small_items_pages_[offset] = res.value(); | ||
else | ||
ec = res.error(); | ||
done.Notify(); | ||
}; | ||
ec = storage.Stash(io::Buffer(page), {}, cb); | ||
|
||
done.Wait(); | ||
if (!ec) | ||
return {}; | ||
} | ||
|
||
small_items_pages_[offset] = page; | ||
return {}; | ||
} |
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.
I really wish to use "virtual" keys to store pages. This would not require having custom code for their serialization, as well custom code for loading and stashing them. We could simply rely on the tiered storage for doing this
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.
Second, a missing optimization is that we don't re-use the offloaded page, we only use it to read from it
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.
The idea would be to create a mapping table between new and old offsets and re-create those offsets in small bins
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.
you can add TODOs to detail your ideas..
Regarding virtual keys, I think it's fine to use a predefined prefix like __df__
for dragonfly specific keys if it helps you. one thing to note - how it works with multiple databases. pages are shared between entities from differrent databases,imho
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.
if it helps you
I want to use the existing code for serialization and offloading, so pages will be "just values" and managed automatically
All in all I don't like my code, but not sure where we can simplify or cut corners |
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.
yeah, this PR is not small :(
src/server/rdb_load.cc
Outdated
@@ -481,6 +484,10 @@ void RdbLoaderBase::OpaqueObjLoader::operator()(const RdbSBF& src) { | |||
pv_->SetSBF(sbf); | |||
} | |||
|
|||
void RdbLoaderBase::OpaqueObjLoader::operator()(const RdbTieredSegment& src) { | |||
CHECK(false) << "unreachable"; |
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.
LOG(FATAL)
// If tiering is enabled, try saving the received page on disk | ||
// Fall back to memory in case of errors | ||
if (EngineShard::tlocal() && EngineShard::tlocal()->tiered_storage()) { | ||
auto& storage = EngineShard::tlocal()->tiered_storage()->BorrowStorage(); | ||
|
||
util::fb2::Done done; | ||
std::error_code ec; | ||
auto cb = [this, offset, &ec, &done](io::Result<tiering::DiskSegment> res) { | ||
if (res.has_value()) | ||
small_items_pages_[offset] = res.value(); | ||
else | ||
ec = res.error(); | ||
done.Notify(); | ||
}; | ||
ec = storage.Stash(io::Buffer(page), {}, cb); | ||
|
||
done.Wait(); | ||
if (!ec) | ||
return {}; | ||
} | ||
|
||
small_items_pages_[offset] = page; | ||
return {}; | ||
} |
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.
you can add TODOs to detail your ideas..
Regarding virtual keys, I think it's fine to use a predefined prefix like __df__
for dragonfly specific keys if it helps you. one thing to note - how it works with multiple databases. pages are shared between entities from differrent databases,imho
Faster small bins serialization by extending the rdb format with RDB_TYPE_TIERED_SEGMENT and RDB_OPCODE_TIERED_PAGE