update PySet_GET_SIZE and similar for free-threaded Python#6230
Conversation
Merging this PR will degrade performance by 10.24%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | list_nth |
8 µs | 8.9 µs | -10.24% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing davidhewitt:atomic-load-macros (b6e338c) with main (c85bc26)
|
CI failure was caused by the atomic function being only available under the version-specific API, |
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
Just a couple nits but otherwise this does match CPython.
| #[cfg(not(any(PyPy, GraalPy)))] | ||
| pub unsafe fn PyList_SET_ITEM(op: *mut PyObject, i: Py_ssize_t, v: *mut PyObject) { | ||
| *(*(op as *mut PyListObject)).ob_item.offset(i) = v; | ||
| *(*_PyList_CAST(op)).ob_item.offset(i) = v; |
There was a problem hiding this comment.
Should we maybe include the debug asserts CPython has?
| return _Py_atomic_load_ssize_relaxed(&(_PyVarObject_CAST(byte_array)->ob_size)); | ||
| #[cfg(not(any(PyPy, GraalPy)))] | ||
| pub unsafe fn PyByteArray_AS_STRING(op: *mut PyObject) -> *mut c_char { | ||
| (*_PyByteArray_CAST(op)).ob_start |
There was a problem hiding this comment.
preexisting issue, but, on 3.14 and older the C macro still returns a pointer to a static empty string when Py_SIZE(self) == 0, and an empty bytearray's ob_start is NULL on those versions:
So on ≤ 3.14 this binding can return NULL where CPython's macro never does. Only matters for raw FFI users, pyo3 doesn't use this.
| @@ -0,0 +1 @@ | |||
| Fix FFI definitions `PyByteArray_GET_SIZE`, `PyList_GET_SIZE`, and `PySet_GET_SIZE` to use an atomic load for free-threaded Python. | |||
There was a problem hiding this comment.
should there maybe also be a release note for PySet_MIN_SIZE being (correctly) moved out of limited API visibility?
3 participants
I got a ping from a Codex security scan after #6226 that the FFI definitions were not implemented properly for free-threaded Python.
I tried to make the shape of the corrected implementations match CPython as much as possible; this required adding some atomic helpers and various
_CASTfunctions.