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

Evaluate unsafe code in encoder #52

Closed
oyvindln opened this issue Jul 8, 2019 · 6 comments
Closed

Evaluate unsafe code in encoder #52

oyvindln opened this issue Jul 8, 2019 · 6 comments

Comments

@oyvindln
Copy link
Collaborator

oyvindln commented Jul 8, 2019

There are still a number of uses of unsafe in the encoder code, more specifically in the match finding code to avoid bounds checks inside tight loops. While we've looked over them before, it would be a good idea to check them again and try to get rid of as many as we can if they have no or negligible effect on performance. In many of the unsafe calls the compiler may already be able to optimize them away anyhow.

The only unsafe code in the decoder are simple bytes to integer conversions, which should be sound.

Once 1.34 is reliably available in distros, we can switch to using the built-in functions for converting from bytes to integers.

@oyvindln
Copy link
Collaborator Author

It looks like the compiler is smart enough to optimize code making a 16-bit value from two consecutive 8-bit values in an array using a | b << 8 to pretty much the same code as when using ptr::unaligned_load so this should let us avoid more unsafe even while staying on 1.32.

I'll see how it plays out in the actual code next.

@oyvindln
Copy link
Collaborator Author

Right, 3b7208b removes most of the remaining unsafe in the encoder. The only two bits now are the unaligned reads, which have bounds checks right before them.

I checked over the other unsafe uses and couldn't find any unsoundness. They were however not well isolated to avoid any code change that could affect the soundness. There was only a marginal performance improvement at best so I just removed it anyhow. We can look into minor safe performance tweaks later.

@Shnatsel
Copy link
Contributor

Sounds like a great time for a point release!

For the record, it should be possible to eliminate unaligned reads as well using the trick from #48, but that requires rustc 1.34.

@Shnatsel
Copy link
Contributor

Debian Stable is now on 1.34, Ubuntu is on 1.35 and Fedora is on 1.36. How do you feel about making a point release immediately, so that people stuck on old compilers could get at least some of the safety improvements, then converting the remaining unaligned loads to safe code and issuing another point release with an MSRV version bump?

@oyvindln
Copy link
Collaborator Author

Sounds good. I've published a new version. Will look at updating the rest later.

@oyvindln
Copy link
Collaborator Author

oyvindln commented Aug 6, 2019

Closing as there is no remaining unsafe code in the core crate.

@oyvindln oyvindln closed this as completed Aug 6, 2019
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

No branches or pull requests

2 participants