-
-
Notifications
You must be signed in to change notification settings - Fork 33.3k
gh-140557: Force alignment of empty bytearray and array.array buffers
#140559
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
base: main
Are you sure you want to change the base?
Conversation
This ensures the buffers used by the empty `bytearray` and `array.array` are aligned the same as a pointer returned by the allocator. This is a more convenient default for interop with other languages that have stricter requirements of type-safe buffers (e.g. Rust's `&[T]` type) even when empty.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. 👍
|
I'm 👎 on this for see: gh-139871 for ways having a |
|
re: |
|
This doesn't enforce that every buffer always has aligned access - you can easily take out a view of a bytes or bytearray that you offset by a byte - so other languages have to still handle the case of an unaligned pointer. It just makes the default empty object aligned at zero cost, and that object is common. The empty |
|
We do still have to handle unaligned access everywhere, I agree there's no escaping it. The goal here is only around making unaligned objects less common - |
|
I'm still concerned here: Both
|
|
I'm not aiming to make this a guarantee at all, just that by default the zero allocation case is already aligned. In Rust and other languages we do still have to handle unaligned buffers, just like Numpy alread has to in pure C.
Right, this is where other languages have stronger guarantees - I mentioned it in gh-140557 as the motivation that in Rust, creating a |
|
About rarity of appearance: both of these pointers show through the buffer protocol like you mentioned, and that's where I come across them in language interop - that's the defined way to get zero-copy access to a data buffer owned by Python space. We can't have zero-copy access to misaligned buffers, but in practice, the vast majority of buffers that are back by an allocation end up aligned anyway, so requiring copies is rare (since you have to deliberately offset an allocated pointer by a sub-unit amount). We can add special handling to produce an empty slice in Rust that doesn't refer to the same pointer we actually get from the Python buffer protocol, if it's misaligned as an extra optimisation to avoid a copy, so we don't require CPython support, but if the default is actually aligned, then there's less point propagating this hypothetical optimising special-handling code through a lot of downstream packages and just using the slower "copy to force alignment" paths that (should) already exist for it. When I wrote this patch, it was zero cost to CPython to achieve that for the defaults. If you have additional work that would seriously raise the cost to CPython then the calculus is different, though even reliably having the empty buffer aligned on an 8, like the empty |
|
Can you link to the code or provide a rust sample which needs to special-case handling a zero-length bytearray? I think that would help me understand here. |
|
It is not guaranteed that the start of the bytearray buffer has some alignment. This is a CPython implementation detail. But some code depends on this, and it may not work on non-x86 platforms if this is not aligned. There are exceptions: if there was a deletion from the beginning of the bytearray (we cannot do anything with this, but the user can guarantee that this did not happen), and when the bytearray is empty. The latter case is easy to fix for us, it costs nothing. |
|
cmaloney: Let's say I've got FFI got that wraps the struct PyBuffer {
buf: *mut (),
itemsize: isize,
ndim: ::std::ffi::c_int,
shape: *const isize,
strides: *const isize,
}
impl PyBuffer {
/// Is the slice contiguous in memory?
fn is_contiguous(&self) -> bool { /* ... */ }
/// How many bytes can be read from it?
fn len_bytes(&self) -> usize { /* ... */ }
}Let's say I've got one of these structs that I then initialised with enum SliceError {
Unaligned,
Noncontiguous,
Nullptr,
}
fn slice_from_buffer<T>(buf: &PyBuffer) -> Result<&[T], SliceError> {
if buf.buf.is_null() {
return Err(SliceError::Nullptr);
}
if !buf.is_contiguous() {
return Err(SliceError::Noncontiguous);
}
if !buf.buf.is_aligned::<T>() {
return Err(SliceError::Unaligned);
}
// SAFETY: pointer is non-null, aligned, and valid for this many contiguous reads:
Ok(unsafe { std::slice::from_raw_parts(
buf.buf,
buf.len_bytes() / std::mem::size_of::<T>()
})
}I have to do the alignment and contiguous checks before I call A Rust caller of this function is still responsible for handling the case of an unaligned pointer, which might cause them to do something like match slice_from_buffer::<u64>(&buf) {
Ok(slice) => use_slice(slice),
Err(SliceError::Noncontiguous | SliceError::Unaligned) => {
let aligned_contiguous = /* copy buffer to somewhere aligned */;
use_slice(aligned_contiguous.as_slice())
}
Err(SliceError::Nullptr) => panic!(),
}The idea of this PR is just to make it so that slightly more stuff by default gets to go down the happy path, in a way that doesn't cost CPython anything. It's still the Rust user's responsibility to handle the unhappy path since that's totally valid Python code still, and the Rust library's responsibility to make sure everything is safe for FFI use in Rust2. This PR isn't intending to add any restrictions on what CPython is allowed to do or what other Python implementations may do. Your #139871 looks to me to also achieve the same goal I was going for here, just as a side effect (since the empty Footnotes
|
|
I'm 👍 for doing
|
cmaloney
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reduce scope to just array.array + add a test; empty buffer + non-empty array.array storage being aligned I think is a nice improvement
|
I do not think that bytearray should be excluded. If not a special optimization of using NULL for empty array, it would have standard alignment (as returned by |
|
Then I do not see reasons to object this change. |
I have slight nit in wording of the NEWS entry; I think it would be good to mention Rust and to make it more concise. |
|
I think there is a good chance that the code that relies on this already exist. It works only because the Intel plathform is tolerable to unaligned access (and there is no actually access to memory here, because it is an empty buffer). But on other platforms pointers of different type can use different registers. This is not limited to Rust. In C, casting a pointer with wrong alignment can be an undefined behavior. If bytesarray is used instead of array as a collection of 16-, 32- or 64-bit integers, there may be problems. |
719e0e2 to
dd9e50b
Compare
|
Hi both - I'd been busy at work the last few days and not had a chance to check in, sorry. Given the above comments: I've added some tests of the new behaviour (open to suggestions if I've missed some API that'd make them simpler), and I had a go at shortening the NEWS entry to make it shorter and stress that it's just about the empty default, not a guarantee for all buffers. Happy to change anything more if there's consensus, or to pause this and take it to Discourse if needs be? I'm definitely not trying to make all buffers follow Rust semantics - that's never possible anyway, given that we can always do |
|
I think we're all moving towards consensus and actually really near it :). Will do a more thorough review later today |
| return _linked_to_musl | ||
|
|
||
|
|
||
| try: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will be easier to do in Lib/test/test_capi/test_misc.py. In particular only need to
- Add a new
_testcapimodule.centry point that makes a Py_buffer C API of the PyObject passed to it, gets the pointer and turns that pointer into aPyObject *(https://docs.python.org/3/c-api/long.html#c.PyLong_FromVoidPtr) which it returns - one alignment test across the range of types and constructions care about.
I like how your existing test iterates through / tests all the different array typecodes.
I think it would be good to extend the test to both test empty (the size that caused this bug) + non-empty arrays (they should also be aligned)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually found a better test file for the alignment pieces to live in: Lib/test/test_buffer.py; still should implement "get the pointer" in _testcapimodule.c
This ensures the buffers used by the empty
bytearrayandarray.arrayare aligned the same as a pointer returned by the allocator. This is a more convenient default for interop with other languages that have stricter requirements of type-safe buffers (e.g. Rust's&[T]type) even when empty.I tried to do the same for
bytes, but I think its default buffer is only forcibly aligned on an 8 because of theuint64_tmember inPyBytesObject, and it ends up dependent on wherebytes_emptygets laid out. If that's desirable too, I might need some help figuring out a strategy for it.I'm not sure where's appropriate to put a test for this, or if it can/should be documented as reliable.
Issue: #140557
bytearrayandarray.array#140557