-
Notifications
You must be signed in to change notification settings - Fork 67
Use Bytes instead of Vec<u8> to reduce memory allocations and improve performance #55
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
base: master
Are you sure you want to change the base?
Conversation
That's amazing! In the write-a-lot example, do you also have numbers regarding the throughput? How many writes per second are enabled by this change compared to the status quo? |
I am writing some Criterion based tests to explain these behaviors — a proper evaluation metric might be needed to measure the following series of changes. The content in the screenshot above was compiled in debug mode, so there are significant differences shown. Although the compiler has drastically reduced the differences between them, I still get some positive feedback while writing the test. This PR is actually a rather superficial attempt. In fact, the data flow involving blockContent, various iterators, internal and memtable are all involved in this issue. This change is suit for those remain unchanged ones written. I need to write a test suite involving different kinds of reads, writes, and whether through snap, etc. But at least now I see the most obvious change is reflected in Iter. Perhaps it will take some time to understand how the data flows, which changes are unnecessary, and to improve the test suit.
|
Performance ImprovementsDB_WriteBatch/Batch1000_Snappy DB_Iteration/Items1000_None DB_Iteration/Items1000_Snappy DB_Iteration/Items10000_None DB_Iteration/Items10000_Snappy DB_Snapshot_Get/None time: [164.00 µs 170.36 µs 177.92 µs] DB_Snapshot_Get/Snappy time: [180.69 µs 184.57 µs 188.30 µs] DB_CompactRange/None time: [6.2198 ms 6.3014 ms 6.3856 ms] DB_CompactRange/Snappy time: [7.9395 ms 8.0341 ms 8.1310 ms] |
11745e1
to
f9ef19c
Compare
Just a first couple comments, but overall good work! I will review the rest hopefully quite soon. |
use integer_encoding::FixedInt; | ||
use integer_encoding::VarInt; | ||
|
||
pub type BlockContents = Vec<u8>; | ||
pub type BlockContents = Bytes; |
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.
can the block
field in struct Block
now just be a bare BlockContents
? IIUC, Bytes
is already cloneable, so does not need to be wrapped in an Rc<>
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 have tried this operation, but it resulted in some strange minor performance degradation. I'm not quite sure about the reason for this. However, from the perspective of Rust's general design philosophy, we should clearly do this.
The CRC transformation significantly improves write performance, and this PR primarily improves iteration performance. So I think the minor performance degradation I considered earlier can no longer be taken into account.
examples/leveldb-tool/src/main.rs
Outdated
@@ -34,7 +34,7 @@ fn iter(db: &mut DB) { | |||
let (mut k, mut v) = (vec![], vec![]); | |||
let mut out = io::BufWriter::new(io::stdout()); | |||
while it.advance() { | |||
it.current(&mut k, &mut v); | |||
it.current(); |
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 believe this line forgets to update k
and v
, meaning the writes below are pointless. The returned values should probably be stored and written, instead.
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 previously focused mainly on the copy overhead of the library and didn't pay much attention to the changes related to the examples. These examples might only have passed clippy without a careful review of the type conversions.
examples/mcpe/src/main.rs
Outdated
@@ -100,5 +100,5 @@ fn main() { | |||
let mut db = DB::open(PATH, opt).unwrap(); | |||
db.put(b"~local_player", b"NBT data goes here").unwrap(); | |||
let value = db.get(b"~local_player").unwrap(); | |||
assert_eq!(&value, b"NBT data goes here") | |||
assert_eq!(&*value, b"NBT data goes 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.
is the &
operator necessary here? value.deref()
should already produce a &[u8]
?
src/filter.rs
Outdated
@@ -139,7 +139,7 @@ impl FilterPolicy for BloomPolicy { | |||
// Add all keys to the filter. | |||
offset_data_iterate(keys, key_offsets, |key| { | |||
let mut h = self.bloom_hash(key); | |||
let delta = (h >> 17) | (h << 15); | |||
let delta = h.rotate_left(15); |
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 equivalent? If not, what's the rationale? In the original implementation, this is supposed to swap the first two bytes and the last two bytes (dropping two bits in the middle), but now, the second two bytes will always be 0x8000 or 0x0000, right?
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.
rust-analyzer complains about this, and I think
(h >> 17) | (h << 15)
= (h << 15) | (h >> 17)
= h.rotate_left(15)
stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/num/uint_macros.rs
explains that
/// Shifts the bits to the left by a specified amount, `n`,
/// wrapping the truncated bits to the end of the resulting integer.
///
/// Please note this isn't the same operation as the `<<` shifting operator!
///
/// # Examples
///
/// Basic usage:
///
/// ```
#[doc = concat!("let n = ", $rot_op, stringify!($SelfT), ";")]
#[doc = concat!("let m = ", $rot_result, ";")]
///
#[doc = concat!("assert_eq!(n.rotate_left(", $rot, "), m);")]
/// ```
A quick check is here
https://gist.github.com/lonless9/5a0a55c67ed773f76f9d9cfdbf8e02e2
Even if changed to this rusty form, I don't think it will provide any advantage or disadvantage—just eliminating one that compiler would complain.
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 think this change related to clippy is not relevant to this PR, and I will revert it. Actually, there are many clippy-related warnings now. Maybe we can focus on cleaning them up next time.
src/version.rs
Outdated
@@ -592,12 +593,20 @@ pub mod testutil { | |||
largest: &[u8], | |||
largestix: u64, | |||
) -> FileMetaHandle { | |||
let smallest_key = LookupKey::new(smallest, smallestix); |
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.
these two variables are unused, you can remove them
6af07a6
to
02a394c
Compare
02a394c
to
0e2afab
Compare
This PR contains serval commits that replace Vec with Bytes to reduce memory allocations and improve performance.
Optimization Rationale
In the original code, extensive use of Vec leads to frequent memory allocations and copying operations, especially in key-value storage systems where these operations occur frequently. By using Bytes, we can: