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

bytes specialization via unsafe slice cast #4423

Closed

Conversation

davidhewitt
Copy link
Member

This is an extension of #4417 to attempt to maximise performance by utilising unsafe slice casting to convert

Code is a bit of a mess but hopefully illustrates the idea.

Benchmarks against PyBytes::new:

bytes_new_small         time:   [3.0873 ns 3.1257 ns 3.1597 ns]
bytes_new_medium        time:   [33.446 ns 33.675 ns 33.884 ns]
bytes_new_large         time:   [7.3872 µs 7.4767 µs 7.5715 µs]

byte_slice_into_pyobject_small
                        time:   [4.0567 ns 4.1082 ns 4.1558 ns]
byte_slice_into_pyobject_medium
                        time:   [36.650 ns 37.367 ns 38.093 ns]
byte_slice_into_pyobject_large
                        time:   [7.7810 µs 7.8967 µs 8.0074 µs]

}
impl<T, const N: usize> crate::conversion::SliceableIntoPyObjectIterator for [T; N] {
fn as_bytes_slice(&self) -> &[u8] {
unsafe { std::slice::from_raw_parts(self.as_ptr().cast::<u8>(), self.len()) }
Copy link
Member Author

@davidhewitt davidhewitt Aug 8, 2024

Choose a reason for hiding this comment

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

These implementations all containing an unsafe slice cast make me uneasy. In theory we can ensure these are only called by us on T = u8, but this does leak into the (hidden) public API so we'd need to be sure it's not unsound to do this in general. Probably need to be careful about alignment and size of T if so.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we can at least make the API public-but-unreachable (like Sealed) so that it can only be used by us internally.

@davidhewitt
Copy link
Member Author

Superseded by #4442

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