Add a blob-specific cache priority (#10461) #354
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: RocksDB's `Cache` abstraction currently supports two priority levels for items: high (used for frequently accessed/highly valuable SST metablocks like index/filter blocks) and low (used for SST data blocks). Blobs are typically lower-value targets for caching than data blocks, since 1) with BlobDB, data blocks containing blob references conceptually form an index structure which has to be consulted before we can read the blob value, and 2) cached blobs represent only a single key-value, while cached data blocks generally contain multiple KVs. Since we would like to make it possible to use the same backing cache for the block cache and the blob cache, it would make sense to add a new, lower-than-low cache priority level (bottom level) for blobs so data blocks are prioritized over them. This task is a part of facebook#10156 Pull Request resolved: facebook#10461 Reviewed By: siying Differential Revision: D38672823 Pulled By: ltamasi fbshipit-source-id: 90cf7362036563d79891f47be2cc24b827482743
Signed-off-by: Connor1996 <[email protected]>
Signed-off-by: Connor1996 <[email protected]>
tonyxuqqi
approved these changes
Jan 30, 2024
v01dstar
approved these changes
Jan 30, 2024
| @@ -163,7 +163,7 @@ TEST_P(BlockBasedTableReaderTest, MultiGet) { | |||
| // Internal key is constructed directly from this key, | |||
| // and internal key size is required to be >= 8 bytes, | |||
| // so use %08u as the format string. | |||
| sprintf(k, "%08u", key); | |||
| snprintf(k, sizeof(k), "%08u", key); | |||
There was a problem hiding this comment.
Were these changes for debugging only? If so, maybe we should consider reverting, unless we want to contribute this PR to the facebook upstream
There was a problem hiding this comment.
Newer compiler complains safety issue
tonyxuqqi
reviewed
Jan 31, 2024
| low_pri_pool_usage_ += total_charge; | ||
| } | ||
|
|
||
| while (low_pri_pool_usage_ > low_pri_pool_capacity_) { |
There was a problem hiding this comment.
I don't understand that why the actual link is not updated, but just update the usage_?
The entry is still with the previous priority link (either lru_ or lru_low_pri_
There was a problem hiding this comment.
Three priorities are composed in one link, like below
A---->B---->C---->D---->E---->F
^ ^ ^
| | |
bottom low high
So when one priority is full, only need to move the pointer of lower priority
tonyxuqqi
reviewed
Jan 31, 2024
| while (low_pri_pool_usage_ > low_pri_pool_capacity_) { | ||
| // Overflow last entry in low-pri pool to bottom-pri pool. | ||
| lru_bottom_pri_ = lru_bottom_pri_->next; | ||
| assert(lru_bottom_pri_ != &lru_); |
There was a problem hiding this comment.
This was referenced Jan 31, 2024
glorv
approved these changes
Feb 1, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
RocksDB's
Cacheabstraction currently supports two priority levels for items: high (used for frequently accessed/highly valuable SST metablocks like index/filter blocks) and low (used for SST data blocks). Blobs are typically lower-value targets for caching than data blocks, since 1) with BlobDB, data blocks containing blob references conceptually form an index structure which has to be consulted before we can read the blob value, and 2) cached blobs represent only a single key-value, while cached data blocks generally contain multiple KVs. Since we would like to make it possible to use the same backing cache for the block cache and the blob cache, it would make sense to add a new, lower-than-low cache priority level (bottom level) for blobs so data blocks are prioritized over them.This task is a part of facebook#10156
Pull Request resolved: facebook#10461
Reviewed By: siying
Differential Revision: D38672823
Pulled By: ltamasi
fbshipit-source-id: 90cf7362036563d79891f47be2cc24b827482743