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

Refine blob cache related code #309

Merged
merged 4 commits into from
Feb 5, 2024
Merged

Conversation

Connor1996
Copy link
Member

@Connor1996 Connor1996 commented Jan 31, 2024

This PR does:

Signed-off-by: Connor1996 <[email protected]>
Signed-off-by: Connor1996 <[email protected]>
@@ -725,10 +724,6 @@ Status TitanDBImpl::GetImpl(const ReadOptions& options,
options.snapshot->GetSequenceNumber(),
s.ToString().c_str());
}
if (s.ok()) {
value->Reset();
value->PinSelf(record.value);
Copy link
Member Author

Choose a reason for hiding this comment

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

Here introduces one memory copy

} else {
buffer->PinSlice(blob, OwnedSlice::CleanupFunc, blob.release(), nullptr);
value->PinSlice(record->value, OwnedSlice::CleanupFunc, blob.release(),
Copy link
Contributor

@tonyxuqqi tonyxuqqi Jan 31, 2024

Choose a reason for hiding this comment

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

So the new logic will return the value only and the old code returns the whole kv value pair?
If that, should the caller of Get() be aware of this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

This does not feel right. BlobRecord::DecodeFrom() assumes the buffer contains both key and value. While, I guess this PR wants to optimize the cache utilization by storing value only. In addition to changing the PR description and func comment, we also need to change the decoding logic of BlobRecord accordingly.

Copy link
Member Author

@Connor1996 Connor1996 Feb 1, 2024

Choose a reason for hiding this comment

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

So the new logic will return the value only and the old code returns the whole kv value pair?

Old code reads the kv value pair by record field. New logic still can read the kv value pair by record field. The record is just a slice refering to the pinnable slice underlying buffer. The value pinnable slice holds the underlying buffer, while as a slice I let it only read out value from it instead of the whole buffer.

Should the caller of Get() be aware of this change?

Previously, caller wouldn't use the buffer slice directly as they don't know the content. Key and value are read by record field. So no need to change and I just change the naming buffer to value

We also need to change the decoding logic of BlobRecord accordingly.

No need. When calling BlobRecord::DecodeFrom(), the buffer doesn't contain both key and value.

Copy link
Contributor

@v01dstar v01dstar left a comment

Choose a reason for hiding this comment

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

LGTM

@Connor1996 Connor1996 merged commit 55f2cf8 into tikv:master Feb 5, 2024
1 of 2 checks passed
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

Successfully merging this pull request may close these issues.

4 participants