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.

Sign up for GitHub

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

Eliminate the copying of blobs when serving reads from the cache #10297

Closed
wants to merge 11 commits into from
Closed

Eliminate the copying of blobs when serving reads from the cache #10297

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c4b8273
Eliminate the copying of blobs when serving reads from the cache
gangliao Jul 1, 2022
10f91dc
Follow the comments
gangliao Jul 1, 2022
6272052
Cleanup
gangliao Jul 5, 2022
3cc692e
Change to static_cast
gangliao Jul 5, 2022
1648db1
Add new TransferTo
gangliao Jul 5, 2022
4514dae
Fix clang-format
gangliao Jul 5, 2022
01c1d8f
Follow the comments
gangliao Jul 5, 2022
008bc9b
Follow the comments
gangliao Jul 6, 2022
558087e
Cleanup
gangliao Jul 6, 2022
4c75c32
Follow the comments
gangliao Jul 6, 2022
3369e7e
Update ReleaseCacheHandle()
gangliao Jul 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions db/blob/blob_source.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,11 @@ Status BlobSource::GetBlob(const ReadOptions& read_options,
Slice key = cache_key.AsSlice();
s = GetBlobFromCache(key, &blob_entry);
if (s.ok() && blob_entry.GetValue()) {
value->PinSelf(*blob_entry.GetValue());
// To avoid copying the cached blob into the buffer provided by the
// application, we can simply transfer ownership of the cache handle to
// the target PinnableSlice. This has the potential to save a lot of
// CPU, especially with large blob values.
blob_entry.TransferTo(value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if there's a better way to do this. We could consider adding a PinnableSlice::PinSlice overload that takes a Slice and a CachableEntry; however, this would involve exposing CachableEntry (which is currently an internal/implementation class) in the public API. So we probably wouldn't want to do that.

What I'm thinking is using the existing PinSlice(const Slice& s, CleanupFunction f, void* arg1, void* arg2) overload (with ReleaseCacheHandle as the fn, and the cache and cache handle as args, like below; note that we're guaranteed to be dealing with a cached value here, so there is no need for an "own value" branch), and then make the CachableEntry relinquish ownership of the handle by calling blob_entry.TransferTo(nullptr);. What do you guys think? Bit of a hack I suppose (and we would definitely want to explain what's happening in a comment) but we wouldn't have to add a TransferTo overload to CachableEntry for just this specific use case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, since there is no "own value" case here, we could also consider moving from CachableEntry to CacheHandleGuard. We would have to add a TransferTo(Cleanable*) method to CacheHandleGuard though b/c it doesn't have one rn

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semi-related to this: we could also think about eliminating the copy that happens when we insert a blob into the cache (i.e. the string::assign that happens in BlobSource::PutBlobIntoCache). This would most likely involve a change to BlobFileReader's interface but that shouldn't be a big deal now that we always read blobs via the BlobSource layer. But this is definitely for later (could be a separate follow-up task)

Copy link
Contributor Author

@gangliao gangliao Jul 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I'm thinking is using the existing PinSlice(const Slice& s, CleanupFunction f, void* arg1, void* arg2) overload (with ReleaseCacheHandle as the fn, and the cache and cache handle as args, like below;

I've thought about it this way before. But ReleaseCacheHandle is private. So we need to expose it, right?
Or, we plan to use a lambda function in here...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, we can use a lambda; I think it's cleaner than taking a dependency on CachableEntry just for the purposes of reusing ReleaseCacheHandle.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM. When I first thought about this, the ReleaseCacheHandle being private was my concern. I am fine with either.

Copy link
Contributor Author

@gangliao gangliao Jul 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could also think about eliminating the copy that happens when we insert a blob into the cache (i.e. the string::assign that happens in BlobSource::PutBlobIntoCache). This would most likely involve a change to BlobFileReader's interface but that shouldn't be a big deal now that we always read blobs via the BlobSource layer. But this is definitely for later (could be a separate follow-up task)

If so, who will take ownership? Oh I see. That's really similar to eliminate the copying of blobs when serving reads

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

todo: an additional stretch task for it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If so, who will take ownership? Oh I see. That's really similar to eliminate the copying of blobs when serving reads

We could look at e.g. how BlockContents or UncompressionDict are used for inspiration. BTW, as part of this additional task, we could also address this TODO: https://github.com/facebook/rocksdb/blob/main/db/blob/blob_source.cc#L68-L69


// For consistency, the size of on-disk (possibly compressed) blob record
// is assigned to bytes_read.
Expand Down Expand Up @@ -265,7 +269,12 @@ void BlobSource::MultiGetBlobFromOneFile(const ReadOptions& read_options,
if (s.ok() && blob_entry.GetValue()) {
assert(req.status);
*req.status = s;
req.result->PinSelf(*blob_entry.GetValue());

// To avoid copying the cached blob into the buffer provided by the
// application, we can simply transfer ownership of the cache handle to
// the target PinnableSlice. This has the potential to save a lot of
// CPU, especially with large blob values.
blob_entry.TransferTo(req.result);

// Update the counter for the number of valid blobs read from the cache.
++cached_blob_count;
Expand Down
2 changes: 2 additions & 0 deletions include/rocksdb/slice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,8 @@ class PinnableSlice : public Slice, public Cleanable {

inline bool IsPinned() const { return pinned_; }

inline void Pin() { pinned_ = true; }
gangliao marked this conversation as resolved.
Show resolved Hide resolved

private:
friend class PinnableSlice4Test;
std::string self_space_;
Expand Down
18 changes: 18 additions & 0 deletions table/block_based/cachable_entry.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,24 @@ class CachableEntry {
ResetFields();
}

void TransferTo(PinnableSlice* slice) {
if (slice) {
slice->data_ = value_->data();
slice->size_ = value_->size();
slice->Pin();
gangliao marked this conversation as resolved.
Show resolved Hide resolved
assert(slice->IsPinned());

if (cache_handle_ != nullptr) {
assert(cache_ != nullptr);
slice->RegisterCleanup(&ReleaseCacheHandle, cache_, cache_handle_);
} else if (own_value_) {
slice->RegisterCleanup(&DeleteValue, value_, nullptr);
}
}

ResetFields();
}

void TransferTo(Cleanable* cleanable) {
if (cleanable) {
if (cache_handle_ != nullptr) {
Expand Down