Repository navigation
Conversation
| #[cfg(not(Py_LIMITED_API))] | ||
| ffi::PyList_SET_ITEM(list.as_ptr(), count, item?.into_ptr()); |
There was a problem hiding this comment.
I wonder, is the critical section potentially always necessary on the free-threaded build due to the ability to fish the list out of the interpreter via GC traversal?
| // Holding the critical section around the whole loop turns each acquisition | ||
| // inside PyList_SetItem into a recursive acquisition, which is cheaper. | ||
| // The outer acquisition only pays off from about four elements. | ||
| let count = if cfg!(all(Py_LIMITED_API, Py_GIL_DISABLED)) && len >= 4 { |
There was a problem hiding this comment.
I wonder if dropping the threshold entirely would be that bad?
| let err = PyList::new(py, (0..len).map(MaybeBad)).unwrap_err(); | ||
| assert!(err.is_instance_of::<PyValueError>(py)); | ||
| } | ||
| assert_eq!(PyList::new(py, (0..2).map(MaybeBad)).unwrap().len(), 2); |
There was a problem hiding this comment.
I am not hugely a fan of coupling the test to the threshold, maybe can just test for all lengths up to 10? We could presumably still comment that varying lengths help cover the various paths. Might depend on the other comments.
|
I looked at this again and at least on my ARM mac, this is actually a slowdown. The path that checks for a recursive lock in CPython ends up being slow slower overall. So I'm going to close this. I think the real fix is to add new APIs to create lists from an existing array of objects. This free-threaded slowdown probably justifies it. Unfortunately that doesn't help abi3t-py315 much but also these sorts of issues were bound to come up rolling out a new feature like this. |
2 participants
See python/cpython#158660 for more context about why I'm looking at this.
CPython enters a critical section inside
PyList_SetItem, even for unshared newly created lists. If we enter our own critical section outside of the loop to initialize the list, we can make the recursive critical section acquisitions insidePyList_SetItema bit faster.On the Linux box where I tested this, the
list_newbenchmark is about 15% faster in abi3t builds under a free-threaded interpreter with this patch. That benchmark creates a list with 50,000 entries and is ~40% faster if In set it to create a list with 256 entries. I measured a small slowdown on the GIL-enabled build, but I think that was due to code size changes and slightly different alignment and layout rather than a real slowdown.