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

Added Rust initialisation of Python-allocated bytes #1074

Merged
merged 8 commits into from
Aug 3, 2020
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
- Add `Python::with_gil` for executing a closure with the Python GIL. [#1037](https://github.com/PyO3/pyo3/pull/1037)
- Implement `Debug` for `PyIterator`. [#1051](https://github.com/PyO3/pyo3/pull/1051)
- Implement type information for conversion failures. [#1050](https://github.com/PyO3/pyo3/pull/1050)
- Add `PyBytes::new_with` and `PyBytes::new_with_uninit` for initialising Python allocated bytes inside Rust. [#1074](https://github.com/PyO3/pyo3/pull/1074)

### Changed
- Exception types have been renamed from e.g. `RuntimeError` to `PyRuntimeError`, and are now only accessible by `&T` or `Py<T>` similar to other Python-native types. The old names continue to exist but are deprecated. [#1024](https://github.com/PyO3/pyo3/pull/1024)
Expand Down
74 changes: 74 additions & 0 deletions src/types/bytes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,60 @@ impl PyBytes {
unsafe { py.from_owned_ptr(ffi::PyBytes_FromStringAndSize(ptr, len)) }
}

/// Creates a new Python bytestring object and a mutable slice to its underlying buffer.
/// The bytestring is uninitialised and must not be read.
///
/// Panics if out of memory.
unsafe fn new_with_uninit_impl(py: Python<'_>, len: usize) -> (&mut [u8], &PyBytes) {
let length = len as ffi::Py_ssize_t;
let pyptr = ffi::PyBytes_FromStringAndSize(std::ptr::null(), length);
// Iff pyptr is null, py.from_owned_ptr(pyptr) will panic
let pybytes = py.from_owned_ptr(pyptr);
let buffer = ffi::PyBytes_AsString(pyptr) as *mut u8;
debug_assert!(!buffer.is_null());
let slice = std::slice::from_raw_parts_mut(buffer, len);
(slice, pybytes)
}

/// Creates a new Python bytestring object.
/// The `init` closure can initialise the bytestring.
///
/// # Safety
Copy link
Member

Choose a reason for hiding this comment

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

We don't need #Safety document for safe functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 533c105

/// * The bytestring is zero-initialised and can be read inside `init`.
///
/// Panics if out of memory.
pub fn new_with<F: Fn(&mut [u8])>(py: Python<'_>, len: usize, init: F) -> &PyBytes {
juntyr marked this conversation as resolved.
Show resolved Hide resolved
let (slice, pybytes) = unsafe { Self::new_with_uninit_impl(py, len) };
#[cfg(feature = "slice_fill")]
{
slice.fill(0);
}
#[cfg(not(feature = "slice_fill"))]
{
slice.iter_mut().for_each(|x| *x = 0);
Copy link
Member

Choose a reason for hiding this comment

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

This is optimized by the compiler and not so slow.
I don't see any clear reason we should use (unstable) fill here. It's actually for loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 533c105

}
init(slice);
pybytes
}

/// Creates a new Python bytestring object.
///
/// # Safety
/// * The bytestring is uninitialised and must not be read inside `init`.
/// * The entire bytestring must be written to inside `init` such that any code that
/// follows does not read uninitialised memory.
///
/// Panics if out of memory.
pub unsafe fn new_with_uninit<F: Fn(&mut [u8])>(
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Member

Choose a reason for hiding this comment

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

If so, the closure should take &mut [MaybeUninit<u8>] as @programmerjake says.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed in 533c105

py: Python<'_>,
len: usize,
init: F,
) -> &PyBytes {
let (slice, pybytes) = Self::new_with_uninit_impl(py, len);
init(slice);
pybytes
}

/// Creates a new Python byte string object from a raw pointer and length.
///
/// Panics if out of memory.
Expand Down Expand Up @@ -91,4 +145,24 @@ mod test {
let bytes = PyBytes::new(py, b"Hello World");
assert_eq!(bytes[1], b'e');
}

#[test]
fn test_bytes_new_with() {
let gil = Python::acquire_gil();
let py = gil.python();
let py_bytes = PyBytes::new_with(py, 10, |b: &mut [u8]| {
b.copy_from_slice(b"Hello Rust");
});
let bytes: &[u8] = FromPyObject::extract(py_bytes).unwrap();
assert_eq!(bytes, b"Hello Rust");
}

#[test]
fn test_bytes_new_with_zero_initialised() {
let gil = Python::acquire_gil();
let py = gil.python();
let py_bytes = PyBytes::new_with(py, 10, |_b: &mut [u8]| ());
let bytes: &[u8] = FromPyObject::extract(py_bytes).unwrap();
assert_eq!(bytes, &[0; 10]);
}
}