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

fix #263 #264

Merged
merged 4 commits into from
May 19, 2023
Merged

fix #263 #264

merged 4 commits into from
May 19, 2023

Conversation

bsbds
Copy link
Collaborator

@bsbds bsbds commented May 9, 2023

fix #263
Please briefly answer these questions:

  • what problem are you trying to solve? (or if there's no problem, what's the motivation for this change?)

    improve the read and write logic for the RocksSnapshot

  • what changes does this pull request make?

    change of SnapshotApi in read_exact and write_all funtions and all associated calls on these funtions

  • are there any non-obvious implications of these changes? (does it break compatibility with previous versions, etc)

    read_exact and write_all funtions now use bytes::{Bytes, BytesMut} as parameter types

@bsbds bsbds marked this pull request as ready for review May 9, 2023 07:16
@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Merging #264 (8626b05) into master (74fee6d) will increase coverage by 0.11%.
The diff coverage is 60.36%.

@@            Coverage Diff             @@
##           master     #264      +/-   ##
==========================================
+ Coverage   51.80%   51.92%   +0.11%     
==========================================
  Files          72       73       +1     
  Lines       12265    12264       -1     
==========================================
+ Hits         6354     6368      +14     
+ Misses       5911     5896      -15     
Impacted Files Coverage Δ
curp/src/server/gc.rs 90.60% <0.00%> (-3.81%) ⬇️
xline/src/client/restore.rs 0.00% <0.00%> (ø)
xline/src/server/watch_server.rs 60.49% <ø> (ø)
xline/src/server/xline_server.rs 0.00% <0.00%> (ø)
xline/src/storage/lease_store/mod.rs 65.83% <ø> (ø)
curp/src/server/curp_node.rs 26.49% <7.01%> (-0.82%) ⬇️
curp/src/server/raw_curp/mod.rs 65.64% <50.00%> (ø)
curp/src/rpc/connect.rs 25.56% <60.00%> (ø)
engine/src/api/snapshot_api.rs 60.00% <60.00%> (ø)
curp/src/server/raw_curp/log.rs 92.47% <66.66%> (-0.24%) ⬇️
... and 13 more

curp/src/rpc/connect.rs Outdated Show resolved Hide resolved
curp/src/server/curp_node.rs Outdated Show resolved Hide resolved
engine/src/snapshot_api/rocks_snapshot.rs Outdated Show resolved Hide resolved
engine/src/snapshot_api/rocks_snapshot.rs Outdated Show resolved Hide resolved
engine/src/snapshot_api/rocks_snapshot.rs Outdated Show resolved Hide resolved
engine/src/rocksdb_engine.rs Show resolved Hide resolved
engine/src/rocksdb_engine.rs Outdated Show resolved Hide resolved
curp/src/rpc/connect.rs Outdated Show resolved Hide resolved
xline/src/client/restore.rs Show resolved Hide resolved
@bsbds
Copy link
Collaborator Author

bsbds commented May 16, 2023

Current implementation in chunk_mut for BytesMut only expand the capacity of the BytesMut when it already full. So it won't automatically expand the capacity because either the read_buf function only called once(unless the buf's capacity is zero), or the condition that the length is smaller than the capacity have already been checked. @themanforfree

    #[inline]
    fn chunk_mut(&mut self) -> &mut UninitSlice {
        if self.capacity() == self.len() {
            self.reserve(64);
        }
        UninitSlice::from_slice(self.spare_capacity_mut())
    }

So we do rely on this specific implementation of BytesMut, if we use impl BufMut in the parameter, the chunk_mut may grow capacity before it gets full. @Phoenix500526

@Phoenix500526
Copy link
Collaborator

The chunk_mut method implemented for Vec<u8> doesn't look very different from the one that BytesMut implemented. FYI.

unsafe impl BufMut for Vec<u8> {
    #[inline]
    fn chunk_mut(&mut self) -> &mut UninitSlice {
        if self.capacity() == self.len() {
            self.reserve(64); // Grow the vec
        }

        let cap = self.capacity();
        let len = self.len();

        let ptr = self.as_mut_ptr();
        unsafe { &mut UninitSlice::from_raw_parts_mut(ptr, cap)[len..] }
    }
}

A Vec<u8> will also grow its capacity when necessary. And a &mut [u8] also implements the BufMut trait, but we don't need to bother this issue.

curp/src/server/curp_node.rs Show resolved Hide resolved
engine/src/proxy.rs Show resolved Hide resolved
@bsbds
Copy link
Collaborator Author

bsbds commented May 17, 2023

Since we refactored the code to use bytes crate, the BytesMut objects are typically used uninitialized. Maybe we should consider change the method name from read_exact to read_buf_exact to match with std::io::Read trait method. https://doc.rust-lang.org/std/io/trait.Read.html#method.read_buf_exact

@mergify mergify bot merged commit bc0bc17 into xline-kv:master May 19, 2023
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.

Refactor: improve the read and write logic for the RocksSnapshot
4 participants