Avoid thread unsafe borrowed references under free-threading #6178
Conversation
There was a problem hiding this comment.
Linking #6164 because it's essentially this. I think I'm fine with the new macro rather than using two levels of the existing one though.
When the details of this are decided, can you add it to the documentation (https://cython.readthedocs.io/en/latest/src/userguide/source_files_and_compilation.html#c-macro-defines) since we've now tried to document these macros.
I wonder if a bunch of duplication could be avoided by defining in the module setup code:
#if CYTHON_AVOID_THREAD_UNSAFE_BORROWED_REFS && PY_VERSION_HEX >= 0x030D0000 // adjust version hex here!
#define __Pyx_PyList_GetItemRef(o, i) Py_GetItemRef(o, i)
#else
#define __Pyx_PyList_GetItemRef(o, i) __Pyx_NewRef(PyList_GET_ITEM(o, i))
#endif
your current version has a lot of duplicated code which could be compacted quite a bit.
|
Note that __Pyx_NewRef is an unsafe macro that uses its argument twice. Thus my preference for an inline function.
That said, maybe __Pyx_NewRef should actually be an inline function these days.
|
|
I've pushed a commit with the changes suggested. My feeling is a more serious cleanup of all of this is needed, which I can do in a follow-up PR. |
lysnikolaou
force-pushed
the
nogil-unsafe-borrowed-references
branch
from
433d51b
to
feb7769
Compare
lysnikolaou
force-pushed
the
nogil-unsafe-borrowed-references
branch
from
3588c6e
to
51e3d51
Compare
Cython/Utility/ObjectHandling.c
Outdated
| if ((!boundscheck) || likely(__Pyx_is_valid_index(wrapped_i, PyList_GET_SIZE(o)))) { | ||
| return __Pyx_PyList_GetItemRef(o, wrapped_i); | ||
| } |
There was a problem hiding this comment.
I've created an issue about this #6191
It seems to me that users might want to be able to opt back in to the unsafe optimized version. Just using the safe version is probably OK for this PR though
…out borrowed refs
| static CYTHON_INLINE PyObject *__Pyx_GetItemInt_List_Fast(PyObject *o, Py_ssize_t i, | ||
| CYTHON_NCP_UNUSED int wraparound, | ||
| CYTHON_NCP_UNUSED int boundscheck) { | ||
| #if CYTHON_ASSUME_SAFE_MACROS && CYTHON_ASSUME_SAFE_SIZE && !CYTHON_AVOID_BORROWED_REFS && !CYTHON_AVOID_THREAD_UNSAFE_BORROWED_REFS |
There was a problem hiding this comment.
Could you avoid the duplication by keeping the templating that was here before and conditionally insert && !CYTHON_AVOID_THREAD_UNSAFE_BORROWED_REFS based on the value of type? I'm no tempita expert though so take that worth a grain of salt.
There was a problem hiding this comment.
Hmm, I also don't know enough about tempita to know whether this is possible. I'll look into it.
Cython/Utility/ModuleSetupCode.c
Outdated
| #define __Pyx_PyList_GetItemRef(o, i) PyList_GetItemRef(o, i) | ||
| static CYTHON_INLINE int __Pyx_PyDict_GetItemRef(PyObject *dict, PyObject *key, PyObject **result) { | ||
| return PyDict_GetItemRef(dict, key, result); | ||
| } |
There was a problem hiding this comment.
Looks like these could both be macros.
| #define __Pyx_PyList_GetItemRef(o, i) PyList_GetItemRef(o, i) | |
| static CYTHON_INLINE int __Pyx_PyDict_GetItemRef(PyObject *dict, PyObject *key, PyObject **result) { | |
| return PyDict_GetItemRef(dict, key, result); | |
| } | |
| #define __Pyx_PyList_GetItemRef(o, i) PyList_GetItemRef(o, i) | |
| #define __Pyx_PyDict_GetItemRef(dict, key, result) PyDict_GetItemRef(dict, key, result) |
| return PyDict_GetItemRef(dict, key, result); | ||
| } | ||
| #else | ||
| #define __Pyx_PyList_GetItemRef(o, i) __Pyx_NewRef(PyList_GET_ITEM(o, i)) |
There was a problem hiding this comment.
Since we're not doing any error checking here, this should depend on CYTHON_ASSUME_SAFE_MACROS and actually also !CYTHON_AVOID_BORROWED_REFS since we're preferring a borrowed reference over a strong returned reference here.
Cython/Utility/ModuleSetupCode.c
Outdated
| #if CYTHON_AVOID_BORROWED_REFS | ||
| #if CYTHON_AVOID_BORROWED_REFS && !CYTHON_AVOID_THREAD_UNSAFE_BORROWED_REFS | ||
| Py_INCREF(varnames_tuple_dedup); |
There was a problem hiding this comment.
This is not correct since we've applied a version check above. In older Pythons, the reference will not be increfed.
I'd move the error check + incref into the #else block above (and duplicate the error check in the #if block).
|
Is this good to go? Any more feedback before it's okay to be merged? Also, on another note, could we maybe do an "intermediate" release since Cython currently does not build with the free-threaded build and people seem to need it? If not, is there a plan for an upcoming release? |
No description provided.