-
Notifications
You must be signed in to change notification settings - Fork 589
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
feat(storage): improve block memory usage #15024
Conversation
…nto li0k/storage_improve_block_memory
Bytes::from(decoded) | ||
|
||
assert_eq!(uncompressed_capacity, decoded.len()); | ||
decoded.freeze() |
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 check the implement of Bytes
and I find that it will not allocate and copy any extra memory when convert Vec
to Bytes
. It just record the length of content. cc @MrCroxx
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 revert it first. It looks same when len == cap
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.
And, Enhanced assert limitations
…nto li0k/storage_improve_block_memory
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.
Please provide the memory usage comparasion of w/wo this PR. 🥰
.read_to_end(&mut decoded) | ||
.map_err(HummockError::decode_error)?; | ||
debug_assert_eq!(decoded.capacity(), uncompressed_capacity); | ||
assert_eq!(read_size, uncompressed_capacity); |
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 debug_assert_eq
is better here.
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.
check equal is enough light for decompress
@@ -704,6 +707,15 @@ impl<W: SstableWriter, F: FilterBuilder> SstableBuilder<W, F> { | |||
data_len, block_meta.offset | |||
) | |||
}); | |||
|
|||
if data_len as usize > self.options.capacity * 2 { |
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 this log necessary to keep?
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 prefer to keep this log for debugging, Therefore, we can quickly identify similar problems, such as not switching block
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.
IMO, that log shouldn't show up under normal circumstances.
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.
LGTM
Merge queue setting changed
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
part of #14949
related to #15022
Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.