Skip to content
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

gh-129817: Use _PyType_HasFeature() to check tp_flags. #130892

Closed
wants to merge 1 commit into from

Conversation

nascheme
Copy link
Member

@nascheme nascheme commented Mar 5, 2025

In the free-threaded build, an atomic load is needed to safely read tp_flags, avoiding data races. Replace bit tests on tp->tp_flags with calls to _PyType_HasFeature(). When multiple bits are being returned as a result of the test, use PyType_GetFlags().

I think some of these flag tests can safely be a normal load rather than an atomic load. However, it seems better to consistently use _PyType_HasFeature() unless we are sure there is no data race possible.

Implementation notes:

  • this is unfortunately quite a lot of code churn because there are a lot of places we read tp_flags directly. I used this command to find all the places: rg -tc -- '->tp_flags \&' and then used editor macros to make the changes.
  • the flag test expression and _PyType_HasFeature() are not always equivalent since the function only returns 0 and 1 while the test expression might care about individual bits. I wrote a replacement _PyType_HasFeature function that checks how many bits are being passed as the feature and then ensured that only one bit is set. I then manually fixed or confirmed cases were this wasn't so. So, I'm fairly sure using _PyType_HasFeature() as this PR does is correct in all cases.
  • I haven't done a performance comparison on a weakly ordered CPU (e.g. ARM). It's possible this change could negatively impact performance. I'll schedule a pyperformance comparison and link it here.

Related PRs: gh-120210 gh-127588

In the free-threaded build, an atomic load is needed to safely read
`tp_flags`, avoiding data races.  Replace bit tests on `tp->tp_flags`
with calls to `_PyType_HasFeature()`.  When multiple bits are being
returned as a result of the test, use `PyType_GetFlags()`.
@nascheme
Copy link
Member Author

nascheme commented Mar 8, 2025

Closing as I don't think this is the correct approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant