-
-
Notifications
You must be signed in to change notification settings - Fork 590
Support freethreading Python #477
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: master
Are you sure you want to change the base?
Conversation
…er than local to a thread.
…e parser's names dict.
…read-only proxy search. Once we know that there are no further proxies referring to the subtree. we don't need locking anymore.
…rite_with()" order and avoid repeated locking of the same source document (since most appended elements will probably come from the same one).
…utes in public cdef classes. See cython/cython#6995
The current state compiles and runs most tests. But it crashes in the threading tests under Py3.14t, e.g. in "lxml.tests.test_threading.ThreadingTestCase.test_concurrent_proxies":
This stress test concurrently creates Element proxy objects for the same C XML tree node, reusing the same instance if it already exists (potentially created in another thread) and creating a new one if not. The C node keeps a pointer (not an owned reference) to its current proxy object, which enables the reuse but also complicates the concurrent deallocation. The first implementation in lxml, twenty years ago, used weak references, but they were waaaayyyy to slow for lxml's need and were quickly replaced by the backpointer scheme. Previously, the instantiation was guarded by the GIL, now I use a PyMutex on the XML Document instance to guard against concurrent proxy access. The proxy deallocation is really tricky then: EDIT: I actually only recently switched from a Cython's @ngoldbaum, is there a guard in CPython against concurrent object finalisation and deallocation? |
@scoder Btw if the check for reusing object code is Py_RECNT(op) == 1, that check is wrong and needs updating to _PyObject_IsUniquelyReferenced |
I'm not sure, but I shared this PR elsewhere to some core devs who might know. |
Thanks for the hint, but I'm not quite sure that's the right condition. return (_Py_IsOwnedByCurrentThread(ob) &&
_Py_atomic_load_uint32_relaxed(&ob->ob_ref_local) == 1 &&
_Py_atomic_load_ssize_relaxed(&ob->ob_ref_shared) == 0); IIUC from the PR that added it, So, if |
Hmm, but the |
Ah, I think what you meant was not the place where the proxy instance is deallocated but the place where the decision is taken whether to use an existing proxy instance as proxy for a C node. In that case as well, the condition is wrong because it ensures a refcount of 1. A proxy instance can also be reused if the refcount is higher than that. (And I really don't care about how many threads create, use and throw away a proxy object – there can only ever be one, globally, for a given C node.) The problem I've been fighting for a while now is that CPython can decide at any time to start the finalisation of a proxy object, and lxml will only learn about it when I already added a refcount check to cdef inline bint hasProxy(xmlNode* c_node) noexcept:
proxy = <cpython.object.PyObject*> c_node._private
return (
# Node has a proxy reference?
proxy is not NULL and
# Proxy reference is not currently deallocating?
cpython.ref._Py_REFCNT(proxy) > 0
) https://github.com/scoder/lxml/blob/ft_python/src/lxml/proxy.pxi#L18-L27 But that still bares a (small but non-zero) risk of the refcount going to zero after checking the refcount with But, there's apparently a refcount check in |
Sorry I pointed to the wrong function. It's The difference is that if you have something on the stack, the refcount is no longer guaranteed to be the "true" refcount by CPython. CPython elides reference counts for certain temporaries on the stack now. |
The first thing that |
Yeah I get your point, I was answering this question:
Basically, Py_REFCNT is no longer "correct" on 3.14 and later. |
Though I don't think |
Ah, |
No description provided.