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

Reduce Usage of Unsafe #86

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

CalebLBaker
Copy link

@CalebLBaker CalebLBaker commented Mar 13, 2021

Remove unnecessary usage of unsafe and replace some unsafe code with equivalent safe code.

The safe implementation of iter::Bytes::slice_skip proposed by this pull request has been benchmarked to be (slightly) slower than the currently used implementation, so if performance is an issue I can revert the changes to that particular method while still keeping my other changes.

shrink cannot be written without using unsafe (there are issues related to temporarily mutably borrowing the slice multiple times simultaneously), so I am ignoring it in this pull request. I might come back later and look at how shrink is used to see if something safer can be used instead.

I also ignored usage of str::from_utf8_unchecked since it seems apparent that whenever it is used in this crate the bytes passed in have already been checked for utf-8 compliance and so checking again would be redundant.

I messed up in my initial replacement of unsafe code;
The safe code I replaced it with at first didn't have equivalent
functionality to the original;
Should be fine this time though;
@seanmonstar
Copy link
Owner

Thanks so much for filing this! I'll take a look through now :)

@seanmonstar
Copy link
Owner

I was curious how the extra bounds checks would perform, so when I run the benchmarks of master vs this PR, here's what I see:

  • master:

    test bench_httparse       ... bench:         569 ns/iter (+/- 103) = 1265 MB/s
    test bench_httparse_short ... bench:          67 ns/iter (+/- 1) = 1014 MB/s
    
  • PR:

    test bench_httparse       ... bench:         701 ns/iter (+/- 67) = 1027 MB/s
    test bench_httparse_short ... bench:          80 ns/iter (+/- 1) = 850 MB/s
    

@CalebLBaker
Copy link
Author

That looks similar to the results that I got from running the benchmarks. I've played around with benchmarking different subsets of the pull request and the greatest portion of the performance difference definitely seems to come from slice_skip.

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.

None yet

2 participants