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.

Sign up for GitHub

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

Jump to bottom

gh-126312: Don't traverse frozen objects on the free-threaded build #126338

Merged
merged 17 commits into from
Nov 15, 2024
Merged

gh-126312: Don't traverse frozen objects on the free-threaded build #126338

merged 17 commits into from
Nov 15, 2024
+54 −5

Conversation

ZeroIntensity
Copy link
Member

@ZeroIntensity ZeroIntensity commented Nov 2, 2024 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Nov 2, 2024
Closed
@ZeroIntensity ZeroIntensity marked this pull request as draft November 2, 2024 18:59
@ZeroIntensity
Copy link
Member Author

Apparently, the fix isn't as stable as I thought. Marking as draft temporarily.

@ZeroIntensity ZeroIntensity marked this pull request as ready for review November 2, 2024 21:37
tomasr8
tomasr8 reviewed Nov 2, 2024
View reviewed changes
Lib/test/test_gc.py Outdated Show resolved Hide resolved
@rruuaanng
Copy link
Contributor

It looks good.

rruuaanng
rruuaanng approved these changes Nov 3, 2024
View reviewed changes
Python/gc_free_threading.c Outdated Show resolved Hide resolved
colesbury
colesbury reviewed Nov 4, 2024
View reviewed changes
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to gc_free_threading.c look good. A few questions about the test case.

I think we also need to update visit_freeze to avoid marking objects with _PyGC_BITS_UNREACHABLE set as frozen.

Lib/test/test_gc.py Outdated Show resolved Hide resolved
Lib/test/test_gc.py Show resolved Hide resolved
Lib/test/test_gc.py Show resolved Hide resolved
@colesbury
Copy link
Contributor

Please also fix visit_freeze to avoid marking objects with _PyGC_BITS_UNREACHABLE

@ZeroIntensity
Copy link
Member Author

Please also fix visit_freeze to avoid marking objects with _PyGC_BITS_UNREACHABLE

Done. I'll see if I can come up with a quick test case. If not, it's probably fine to merge that untested, as it's such a small change.

vstinner
vstinner reviewed Nov 6, 2024
View reviewed changes
Python/gc_free_threading.c Outdated Show resolved Hide resolved
colesbury
colesbury approved these changes Nov 6, 2024
View reviewed changes
Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM

vstinner
vstinner approved these changes Nov 15, 2024
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I was surprised by the "_PyGC_Freeze() no longer freezes unreachable objects" change which is not "directly" related to "Don't traverse frozen objects on the free-threaded build". But this change makes sense.

@vstinner vstinner merged commit d4c72fe into python:main Nov 15, 2024
43 checks passed
@miss-islington-app
Copy link

Thanks @ZeroIntensity for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@miss-islington-app
Copy link

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

cherry_picker d4c72fed8cba8e15ab7bb6c30a92bc9f2c8f0a2c 3.13

@vstinner
Copy link
Member

@ZeroIntensity: Oh, the automated backport to 3.13 failed. Do you want to attempt to fix the cherry-pick conflict?

@vstinner
Copy link
Member

Merged, thanks @ZeroIntensity.

ZeroIntensity added a commit to ZeroIntensity/cpython that referenced this pull request Nov 15, 2024
@ZeroIntensity @skirpichev
[3.13] pythongh-126312: Don't traverse frozen objects on the free-thr…
780e19d 
…eaded build (pythonGH-126338)

Also, _PyGC_Freeze() no longer freezes unreachable objects.

(cherry picked from commit d4c72fe)

Co-authored-by: Peter Bierma <[email protected]>
Co-authored-by: Sergey B Kirpichev <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Nov 15, 2024

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

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Nov 15, 2024
vstinner pushed a commit that referenced this pull request Nov 15, 2024
@ZeroIntensity @skirpichev
[3.13] gh-126312: Don't traverse frozen objects on the free-threaded …
ecda3ae 
…build (GH-126338) (#126866)

* Fix merge conflicts.

* [3.13] gh-126312: Don't traverse frozen objects on the free-threaded build (GH-126338)

Also, _PyGC_Freeze() no longer freezes unreachable objects.

(cherry picked from commit d4c72fe)

Co-authored-by: Peter Bierma <[email protected]>
Co-authored-by: Sergey B Kirpichev <[email protected]>

---------

Co-authored-by: Sergey B Kirpichev <[email protected]>
@ZeroIntensity ZeroIntensity deleted the fix-nogil-frozen-objects branch November 15, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rruuaanng rruuaanng rruuaanng approved these changes

@skirpichev skirpichev skirpichev left review comments

@colesbury colesbury colesbury approved these changes

@vstinner vstinner vstinner approved these changes

@tomasr8 tomasr8 Awaiting requested review from tomasr8

Assignees

@ZeroIntensity ZeroIntensity

Labels
topic-free-threading
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ZeroIntensity @rruuaanng @colesbury @vstinner @skirpichev @tomasr8