gh-127271: Replace use of PyCell_GET/SET#127272
Merged
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
nascheme changed the title 
These macros are not safe to use in the free-threaded build. Use `PyCell_GetRef()` and `PyCell_SetTakeRef()` instead. Add `PyCell_GET` to the free-threading howto table of APIs that return borrowed refs. Add critical sections to `PyCell_GET` and `PyCell_SET`.
naschemeforce-pushed the gh-127271-cell-get-set branch from ac281e2 to 10ac6e6Comparenascheme requested review from ericsnowcurrently and markshannon as code owners
MemberAuthor
nascheme commented Nov 27, 2024
While tinkering with the cellobject code, I found a couple more data races. I've included the fixes for those in this PR. For the reviewer, the reference counting changes to |
Avoid the type error and return results as before.
mpage reviewed Dec 3, 2024
Contributor
mpage left a comment
mpage left a comment There was a problem hiding this comment.
I kicked off benchmark runs for this change for both the default and free-threaded builds. I'll post the results here once they're ready, likely tomorrow morning.
Uh oh!
There was an error while loading. Please reload this page.
mpage added the 🔨 test-with-refleak-buildbots 
bedevere-bot commented Dec 3, 2024
bedevere-bot removed the 🔨 test-with-refleak-buildbots 
Contributor
mpage commented Dec 3, 2024
Performance on the default build looks neutral. |
nascheme merged commit fc5a0dc into python:main 37 checks passed
Uh oh!
There was an error while loading. Please reload this page.
bedevere-bot commented Dec 3, 2024
|
MemberAuthor
nascheme commented Dec 3, 2024
I can't reproduce those two failing tests on my Debian dev machine. My guess is the failure is not related to this commit. I'll investigate some more though. |
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
* Replace uses of `PyCell_GET` and `PyCell_SET`. These macros are not safe to use in the free-threaded build. Use `PyCell_GetRef()` and `PyCell_SetTakeRef()` instead. * Since `PyCell_GetRef()` returns a strong rather than borrowed ref, some code restructuring was required, e.g. `frame_get_var()` returns a strong ref now. * Add critical sections to `PyCell_GET` and `PyCell_SET`. * Move critical_section.h earlier in the Python.h file. * Add `PyCell_GET` to the free-threading howto table of APIs that return borrowed refs. * Add additional unit tests for free-threading.
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
* Replace uses of `PyCell_GET` and `PyCell_SET`. These macros are not safe to use in the free-threaded build. Use `PyCell_GetRef()` and `PyCell_SetTakeRef()` instead. * Since `PyCell_GetRef()` returns a strong rather than borrowed ref, some code restructuring was required, e.g. `frame_get_var()` returns a strong ref now. * Add critical sections to `PyCell_GET` and `PyCell_SET`. * Move critical_section.h earlier in the Python.h file. * Add `PyCell_GET` to the free-threading howto table of APIs that return borrowed refs. * Add additional unit tests for free-threading.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These macros are not safe to use in the free-threaded build. Use
PyCell_GetRef()andPyCell_SetTakeRef()instead. AddPyCell_GETto the free-threading howto table of APIs that return borrowed refs. Add critical sections toPyCell_GETandPyCell_SET.Fix some thread safety issues with cell objects:
cell_richcompare,cell_repr,cell_set_contentsneeded some work. Add an addtional unit test module that tries to trigger data races for these.