Use PyWeakref_GetRef and critical section in BlockValuesRefs #60540
Conversation
lysnikolaou
commented
Dec 11, 2024
lysnikolaou
commented
- Related to ENH: Python 3.13 free-threading support #59057
- All code checks passed.
lysnikolaou
force-pushed
the
lock-block-values-refs
branch
from
4e28feb to
e21c3c9
Compare
rhshadrach
added
Enhancement
Build
|
Perf impact? |
|
IIUC we'll need to wait for the next cython release based on your PR cython/cython#6538? |
|
I've updated this PR so that it's not blocked on a Cython release. A review would be very helpful. The failing CI jobs seem to be unrelated: The pyodide one fails on main as well, and the manylinux one seems to be related to a docker fail, will probably be okay if rerun. |
| @@ -45,8 +45,12 @@ else | |||
| endif | |||
|
|
|||
| cy = meson.get_compiler('cython') | |||
| cdata = configuration_data() | |||
There was a problem hiding this comment.
Can you move the configuration_data next to the configure_file call? I assume here you did it to avoid repeating the cy.version() check, but I think it reads easier to keep this all together
| new_referenced_blocks = [] | ||
| for ref in self.referenced_blocks: | ||
| status = PyWeakref_GetRef(ref, &pobj) | ||
| if status == 1: |
There was a problem hiding this comment.
So if the reference is dead we are intentionally doing nothing here right? We don't need to handle that at all?
(I'm not very familiar with the referencing here)
| IF CYTHON_COMPATIBLE_WITH_FREE_THREADING: | ||
| new_referenced_blocks = [] | ||
| for ref in self.referenced_blocks: | ||
| status = PyWeakref_GetRef(ref, &pobj) |
There was a problem hiding this comment.
| status = PyWeakref_GetRef(ref, &pobj) | |
| cdef PyObject *status = PyWeakref_GetRef(ref, &pobj) |
Can you just declare / define all in one go? I don't think we need separate blocks for those (same comment for the status variable)
