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

Use copy_from_slice() over clone_from_slice() for u8 slice copies #33536

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

steviez
Copy link
Contributor

@steviez steviez commented Oct 5, 2023

Problem

clone_from_slice() would hypothetically visit each item in the slice and clone it whereas copy_from_slice() can memcpy the whole slice in one go.

Technically, Rust does the right thing for us by making clone_from_slice() defer to copy_from_slice() for types that implement Copy trait. However, we should still use the more efficient method directly to show intent.

Summary of Changes

Use copy_from_slice() over clone_from_slice() for &[u8] copies.

As mentioned, Rust made it such that copy_from_slice() would get called in this instance starting from v1.52.0 as evidenced in this commit: rust-lang/rust@130fb243bd9e

clone_from_slice() would hypothetically visit each item in the slice and
clone it whereas copy_from_slice() can memcpy the whole slice in one go.

Technically, Rust does the right thing for us by making
clone_from_slice() defer to copy_from_slice() for types that implement
Copy trait. However, we should still use the more efficient method
directly to show intent.
@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Merging #33536 (d3acfeb) into master (144e6d6) will decrease coverage by 0.1%.
Report is 17 commits behind head on master.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##           master   #33536     +/-   ##
=========================================
- Coverage    81.7%    81.7%   -0.1%     
=========================================
  Files         802      802             
  Lines      217872   217872             
=========================================
- Hits       178146   178127     -19     
- Misses      39726    39745     +19     

Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@steviez steviez merged commit 402e9a5 into solana-labs:master Oct 5, 2023
16 checks passed
@steviez steviez deleted the bstore_copy_slice branch October 5, 2023 18:13
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.

2 participants