Skip to content

Remove unsafe in with_nix_path() for [u8] #1583

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 21, 2021
Merged

Remove unsafe in with_nix_path() for [u8] #1583

merged 1 commit into from
Dec 21, 2021

Conversation

@rtzoeller
Copy link
Collaborator

@rtzoeller rtzoeller commented Oct 23, 2021

Remove use of unsafe in the implementation of with_nix_path() for [u8]. This also comes with a nice determinism win across input sizes, and is fairly performance neutral (slightly slower for small strings, much faster for large strings).

I suspect the performance degradation in the existing implementation is related to the following note in the CStr::from_ptr() documentation:

Note: This operation is intended to be a 0-cost cast but it is currently implemented with an up-front calculation of the length of the string. This is not guaranteed to always be the case.

Tested with cargo 1.57.0-nightly (7fbbf4e8f 2021-10-19), with variations of the following benchmarking code:

#[bench]
fn bench_with_nix_path_1024(b: &mut test::Bencher) {
    let bytes = std::hint::black_box([70u8; 1024]);
    b.iter(|| {
        bytes.with_nix_path(|cstr| {
            std::hint::black_box(&cstr);
        }).unwrap();
    })
}
Length Before Change After Change
16 37 ns/iter (+/- 0) 44 ns/iter (+/- 0)
64 39 ns/iter (+/- 0) 44 ns/iter (+/- 0)
256 84 ns/iter (+/- 0) 48 ns/iter (+/- 0)
1024 232 ns/iter (+/- 1) 50 ns/iter (+/- 1)
4095 796 ns/iter (+/- 8) 62 ns/iter (+/- 2)

@asomers asomers added this to the 0.24.0 milestone Nov 17, 2021
asomers
asomers approved these changes Nov 17, 2021
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Looks great!

@asomers
Copy link
Member

asomers commented Dec 21, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 21, 2021

Build succeeded:

@bors bors bot merged commit 4f7119c into nix-rust:master Dec 21, 2021
@rtzoeller rtzoeller mentioned this pull request Feb 3, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asomers asomers asomers approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.24.0

Development

Successfully merging this pull request may close these issues.

2 participants

@rtzoeller @asomers