Skip to content

bpo-44881: Integrate GC untrack into trashcan begin. - #27718

Closed
nascheme wants to merge 3 commits into
python:mainfrom
nascheme:trashcan_begin_gc_untrack
Closed

bpo-44881: Integrate GC untrack into trashcan begin.#27718
nascheme wants to merge 3 commits into
python:mainfrom
nascheme:trashcan_begin_gc_untrack

Conversation

@nascheme

@naschemenascheme commented Aug 10, 2021•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

For subtle reasons, PyObject_GC_UnTrack() function must be called before
Py_TRASHCAN_BEGIN(). There have been a number of bugs over the years
related to not doing this particular dance just right. Integrating
the PyObject_GC_UnTrack() call makes it harder to do things incorrectly.
That avoids some hard to find bugs (e.g. only triggered when object
nesting gets deep enough).

Extensions that still call PyObject_GC_UnTrack() explictly will
work correctly but the call is unneeded after this change. It would
still be needed for the extension to work correctly with older versions
of Python.

https://bugs.python.org/issue44881

@nascheme
Integrate GC untrack into trashcan begin.
d045bb6
For subtle reasons, PyObject_GC_UnTrack() function must be called before
Py_TRASHCAN_BEGIN(). There have been a number of bugs over the years
related to not doing this particular dance just right. Integrating
the PyObject_GC_UnTrack() call makes it harder to do things incorrectly.
That avoids some hard to find bugs (e.g. only triggered when object
nesting gets deep enough).
Extensions that still call PyObject_GC_UnTrack() explictly will still
work correctly but the call is unneeded after this change. It would
still be needed for the extension to work correctly with older versions
of Python.
@nascheme
nascheme requested a review from tim-oneAugust 10, 2021 21:50
@pablogsal

pablogsal commented Aug 10, 2021

Copy link
Copy Markdown
Member

Let's land #27678 first to include that change in this PR

@nascheme

nascheme commented Aug 10, 2021

Copy link
Copy Markdown
MemberAuthor

Just to be clear, I don't consider this PR a bugfix and it would not be appropriate to include in 3.10.

@pablogsal

pablogsal commented Aug 10, 2021

Copy link
Copy Markdown
Member

Just to be clear, I don't consider this PR a bugfix and it would not be appropriate to include in 3.10.

Yep, we are on the same page :)

@nascheme
small cleanup, move declarations after
42a10b4
It seems slightly cleaner to have the BEGIN/END macros at the start and
end of the dealloc function body.
vstinner
vstinner reviewed Aug 12, 2021
View reviewed changes
Comment threadObjects/object.c
@ambv

ambv commented Aug 16, 2021

Copy link
Copy Markdown
Contributor

We won't be merging this as of yet because an alternative solution is evaluated in GH-27738. I added the assert Victor wanted so that the benchmarks are fairer between the two approaches.

@github-actions

github-actionsBot commented Sep 16, 2021

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actionsgithub-actionsBot added the stale Stale PR or inactive for long period of time. label Sep 16, 2021
@rhettinger
rhettinger removed their request for review May 3, 2022 06:19
@github-actionsgithub-actionsBot removed the stale Stale PR or inactive for long period of time. label Aug 11, 2022
@naschemenascheme mentioned this pull request Nov 28, 2023
Open
@nascheme

nascheme commented Apr 8, 2025

Copy link
Copy Markdown
MemberAuthor

This PR is out of date and will not be required if GH-132280 gets merged. That PR integrates the trashcan mechanism into Py_Dealloc and also stops using the GC next/prev pointers to store the queue of trash. So, it removes the need to call PyObject_GC_UnTrack() at the exact right time.

@naschemenascheme closed this Apr 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinnervstinnervstinner left review comments
@pablogsalpablogsalpablogsal approved these changes
@1st11st1Awaiting requested review from 1st11st1 is a code owner
@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner
@methanemethaneAwaiting requested review from methanemethane is a code owner
@tim-onetim-oneAwaiting requested review from tim-one

Assignees

No one assigned

Labels

awaiting mergeDO-NOT-MERGE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@nascheme@pablogsal@ambv@vstinner@the-knights-who-say-ni@ezio-melotti@bedevere-bot