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

Add safe version of read_u16_le() that is exactly as fast #48

Closed
wants to merge 1 commit into from

Conversation

Shnatsel
Copy link
Contributor

Add safe version of read_u16_le() that is exactly as fast as unsafe implementations, at least on x86_64.

Sadly, it only works on Rust 1.34 and later due to use of TryFrom trait, so it is commented out for now.

I am not aware of any conditional compilation support based on rustc version, other than https://github.com/dtolnay/select-rustc which pulls in syn as a dependency which considerably increases compilation time in release mode.

@Shnatsel
Copy link
Contributor Author

https://github.com/cuviper/autocfg is be a syn-less way to check for compiler features (it uses build.rs) but I have almost zero experience with build.rs so I will not integrate it myself

@oyvindln
Copy link
Collaborator

I don't know whether 1.32 is old enough to be fine to require. But if so, we should replace all these.

Did you also check the performance of doing it via a bit-shift like in the big-endian version? Maybe the compiler would be smart enough to optimize that now.

@Shnatsel
Copy link
Contributor Author

This actually requires 1.34 because of TryInto. I don't think it's old enough to require yet, but will come to Debian Stable this year, at which point I think it will be fine to require it.

I did not check the big-endian version since I do not have any big-endian hardware. But I'd expect u16::from_le_bytes to perform well on big-endian, or at least as good as anything we can hand-roll.

@oyvindln
Copy link
Collaborator

I meant to see if implementing it with bit shifts rather than reading a [u8;2] as a u16 would be as fast on little endian. On big endian we already do it with bit shifts so I expect u16::from_le_bytes to be at least as fast if not faster.

@oyvindln
Copy link
Collaborator

oyvindln commented Jul 8, 2019

Debian stable just had a new release, and now has rust 1.34, so it would probably be fine to adopt the new features and avoid some more unsafe code.

@Shnatsel
Copy link
Contributor Author

Shnatsel commented Jul 8, 2019

Ubuntu LTS is still stuck on 1.32 though, and this PR requires 1.34

@oyvindln
Copy link
Collaborator

Closing this as I removed the unsafe usage in this function in the last commit without needing try_into.

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