-
Notifications
You must be signed in to change notification settings - Fork 590
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
Changes from 3 commits
20e1c81
14ae42a
cd5c51f
bcb9393
1457d8e
e800b94
4900a4b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -240,7 +240,6 @@ impl<W: SstableWriter, F: FilterBuilder> SstableBuilder<W, F> { | |
self.add(full_key, value).await | ||
} | ||
|
||
/// only for test | ||
pub fn current_block_size(&self) -> usize { | ||
self.block_builder.approximate_len() | ||
} | ||
|
@@ -344,6 +343,12 @@ impl<W: SstableWriter, F: FilterBuilder> SstableBuilder<W, F> { | |
|| !user_key(&self.raw_key).eq(user_key(&self.last_full_key)); | ||
let table_id = full_key.user_key.table_id.table_id(); | ||
let is_new_table = self.last_table_id.is_none() || self.last_table_id.unwrap() != table_id; | ||
let current_block_size = self.current_block_size(); | ||
let is_block_full = current_block_size >= self.options.block_capacity | ||
|| (current_block_size > self.options.block_capacity / 4 * 3 | ||
&& current_block_size + self.raw_value.len() + self.raw_key.len() | ||
> self.options.block_capacity); | ||
|
||
if is_new_table { | ||
assert!( | ||
could_switch_block, | ||
|
@@ -356,10 +361,7 @@ impl<W: SstableWriter, F: FilterBuilder> SstableBuilder<W, F> { | |
if !self.block_builder.is_empty() { | ||
self.build_block().await?; | ||
} | ||
} else if is_new_user_key | ||
&& self.block_builder.approximate_len() >= self.options.block_capacity | ||
&& could_switch_block | ||
{ | ||
} else if is_block_full && could_switch_block { | ||
self.build_block().await?; | ||
} | ||
self.last_table_stats.total_key_count += 1; | ||
|
@@ -705,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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. IMO, that log shouldn't show up under normal circumstances. |
||
tracing::warn!( | ||
"WARN unexpected block size {} table {:?}", | ||
data_len, | ||
self.block_builder.table_id() | ||
); | ||
} | ||
|
||
self.block_builder.clear(); | ||
Ok(()) | ||
} | ||
|
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 convertVec
toBytes
. It just record the length of content. cc @MrCroxxThere 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