use copy_from_slice over write_all #4437
Merged
Conversation
This file contains hidden or 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
WalkthroughThis pull request streamlines how data is written into byte slices in two modules. In the consensus module, the function setting the header’s nonce now directly copies the little-endian bytes for both the nonce and additional data into the header. In the stratum module, updating the minting blob in the job response now directly copies the worker ID buffer into a slice, replacing the previous mutable borrow and write operations. No functional or public interface changes are made. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Function Caller
participant SetHeader as set_header_nonce
participant Header as Header Array
Caller->>SetHeader: Call set_header_nonce(nonce, extra)
SetHeader->>Header: Copy nonce bytes via copy_from_slice
SetHeader->>Header: Copy extra data via copy_from_slice
SetHeader-->>Caller: Return updated header
sequenceDiagram
participant Caller as Function Caller
participant FromMethod as StratumJobResponse::from
participant MintingBlob as Minting Blob
Caller->>FromMethod: Call from(worker_id)
FromMethod->>MintingBlob: Copy worker_id.buff using copy_from_slice
FromMethod-->>Caller: Return StratumJobResponse instance
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
consensus/src/lib.rs (1)
4-11: Consider updating import statementSince the refactoring has removed the usage of
WriteBytesExtmethods, you could also remove the related import:-use byteorder::{LittleEndian, WriteBytesExt}; +use byteorder::LittleEndian;The
LittleEndiantype is still used elsewhere in the file (e.g., line 114), so it should be kept.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
consensus/src/lib.rs(1 hunks)stratum/src/rpc.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Generate code coverage report
🔇 Additional comments (2)
stratum/src/rpc.rs (1)
268-268: Cleaner implementation using copy_from_sliceThe change from using
borrow_mut().write_all(&worker_id.buff)tocopy_from_slice(&worker_id.buff)is a good improvement. This simplifies the code by eliminating the need for mutable borrowing and the associated Write trait operations, making it more idiomatic Rust.The operation is more direct and likely more efficient for this simple buffer copying scenario, as it avoids the additional abstraction layers and potential error handling of the Write trait.
consensus/src/lib.rs (1)
51-52: Code simplification with direct slice operationsThis change replaces the previous implementation that used
WriteBytesExtmethods (write_u32andwrite_all) with direct slice operations viacopy_from_slice. This is a good refactoring because:
- It eliminates intermediate mutable references and method calls
- Makes the code more concise and easier to understand
- Potentially improves performance by using a more direct approach to memory copying
The implementation correctly handles both the extra bytes and the little-endian conversion of the nonce value, maintaining the original functionality.
Benchmark for 8353f6fClick to view benchmark
|
Benchmark for 2c7a21dClick to view benchmark
|
simonjiao
pushed a commit
that referenced
this pull request
Mar 26, 2025
* use copy_from_slice over write_all * fix cargo check
simonjiao
pushed a commit
that referenced
this pull request
Apr 7, 2025
* use copy_from_slice over write_all * fix cargo check
simonjiao
added a commit
that referenced
this pull request
Apr 7, 2025
* use rust 1.82.0 and add storage2/executor2 * Revert "select default vm executor to fix tests (#4435)" * Revert "split transactions to different vm (#4432)" * fix building error * init storage2 when booting * add executor2 * use copy_from_slice over write_all (#4437) * use copy_from_slice over write_all * fix cargo check * fix rust upate warning (#4446) * fix compiler warning * fix commit * update clippy * update rust * fix rust update warning * remove unused manifest keys * remove unncessary default-features * in flavor of config.toml --------- Co-authored-by: nk_ysg <[email protected]>
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.
Pull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Other information
Summary by CodeRabbit