gh-135607: Remove null checking of weakref list in dealloc of extension modules and objects

[xuantengh]

• Issue: gh-135607

[xuantengh]

So next we should:

1. remove the introduced test
2. find all patterns with null checks followed by PyObject_ClearWeakRefs, e.g.:

https://github.com/python/cpython/blob/725da50520c5cc3002f6f55bf67c56853507c604/Modules/_threadmodule.c#L1534-L1542

https://github.com/python/cpython/blob/725da50520c5cc3002f6f55bf67c56853507c604/Modules/_threadmodule.c#L1364-L1370

[rhettinger]

Is this going to create an additional cost for deallocating every instance of every extension module type even if weakrefs are never used?

Does this need a macro with an #ifdef so as to not impact the performance of a no-gil build?

[xuantengh]

Is this going to create an additional cost for deallocating every instance of every extension module type even if weakrefs are never used?

For objects without weakref, the "additional cost" would be: https://github.com/python/cpython/blob/140731ff671395fb7a869c2784429c14dc83fb27/Objects/weakrefobject.c#L1010-L1016 which IMHO is negligible?

[ZeroIntensity]

Is this going to create an additional cost for deallocating every instance of every extension module type even if weakrefs are never used?

PyObject_ClearWeakRefs does the same check, just with an atomic load under free-threading, and a non-atomic load under the default build. I don't think we'll see any performance difference.

There's some project "cost" for adding code, including new tests both in maintenance, readability, and CI time. These sorts of tests are so specific that they are unlikely to catch any future bugs; I don't think they are worthwhile.

Ok, for clarification, when do you want free-threading changes to include tests? I think that a lot of them could go under the "trivial" umbrella, but we've still added tests for them in the past. (It makes sense to me to omit tests now, because we're changing lots of different parts of the codebase, but I don't totally see why it wasn't a good idea for the original one-line change.)

[rhettinger]

I recommend using a macro that restores the NULL check for nogil builds. ISTM there is no reason to hurt the nogil builds, even slightly.

[xuantengh]

https://github.com/python/cpython/blob/c8c13f8036051c2858956905769b7b9dfde4bbd7/Modules/_threadmodule.c#L1534-L1548 https://github.com/python/cpython/blob/c8c13f8036051c2858956905769b7b9dfde4bbd7/Modules/itertoolsmodule.c#L1017-L1025

Converting these two cases will lead to bad internal calls in PyObject_ClearWeakRefs, I believe it's because the ref count of op is 0.

[kumaraditya303]

https://github.com/python/cpython/blob/c8c13f8036051c2858956905769b7b9dfde4bbd7/Modules/_threadmodule.c#L1534-L1548

The weakrefs should be cleared after untracking it from gc, all other places does it similarly.

https://github.com/python/cpython/blob/c8c13f8036051c2858956905769b7b9dfde4bbd7/Modules/itertoolsmodule.c#L1017-L1025

Converting these two cases will lead to bad internal calls in PyObject_ClearWeakRefs, I believe it's because the ref count of op is 0.

Weakrefs should be cleared in tp_dealloc not tp_clear, looks like a bug to me which should be fixed separately.

[miss-islington-app[bot]]

Thanks @xuantengh for the PR, and @kumaraditya303 for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. 🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

[miss-islington-app[bot]]

Sorry, @xuantengh and @kumaraditya303, I could not cleanly backport this to 3.14 due to a conflict. Please backport using cherry_picker on command line.

cherry_picker b1056c2a446b43452e457d5fd5f1bde66afd3883 3.14

[miss-islington-app[bot]]

Sorry, @xuantengh and @kumaraditya303, I could not cleanly backport this to 3.13 due to a conflict. Please backport using cherry_picker on command line.

cherry_picker b1056c2a446b43452e457d5fd5f1bde66afd3883 3.13

[bedevere-app[bot]]

GH-136119 is a backport of this pull request to the 3.14 branch.

[bedevere-app[bot]]

GH-136126 is a backport of this pull request to the 3.13 branch.

[vstinner]

Thanks @xuantengh. I backported your change manually to 3.13 and 3.14 to fix merge conflicts.

[xuantengh]

Thanks @xuantengh. I backported your change manually to 3.13 and 3.14 to fix merge conflicts.

Thanks for your efforts!