GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-44897: WIP: Integrate trashcan into _Py_Dealloc by nascheme · Pull Request #27738 · python/cpython

Conversation

@nascheme

@nascheme nascheme commented Aug 12, 2021 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This is a WIP/proof-of-concept of doing away with Py_TRASHCAN_BEGIN and
Py_TRASHCAN_END and instead integrating the functionality into
_Py_Dealloc. There are a few advantages:

  • all container objects have the risk of overflowing the C stack if a
    long reference chain of them is created and then deallocated. So, to
    be safe, the tp_dealloc methods for those objects should be protected
    from overflowing the stack.

  • the Py_TRASHCAN_BEGIN and Py_TRASHCAN_END macros are hard to
    understand and a bit hard to use correctly. Making the mechanism
    internal avoids confusion. The code can be slightly simplified as
    well.

This proof-of-concept seems to pass tests but it will need some careful
review. The exact rules related to calling GC Track/Untrack are subtle
and this changes things a bit. I.e. tp_dealloc is called with GC
objects already untracked. For 3rd party extensions, they are calling
PyObject_GC_UnTrack() and so I believe they should still work.

The fact that PyObject_CallFinalizerFromDealloc() wants GC objects to
definitely be tracked is a bit of a mystery to me (there is an assert to
check that). I changed the code to track objects if they are not
already tracked but I'm not sure that's correct.

There could be a performance hit, due to the _PyType_IS_GC() test that
was added to the _Py_Dealloc() function. For non-GC objects, that's
going to be a new branch and I'm worried it might hurt a bit. OTOH,
maybe it's just in the noise. Profiling will need to be done.

https://bugs.python.org/issue44897

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@nascheme
Integrate trashcan into _Py_Dealloc.
c3012a0
This is a WIP/proof-of-concept of doing away with Py_TRASHCAN_BEGIN and
Py_TRASHCAN_END and instead integrating the functionality into
_Py_Dealloc.  There are a few advantages:

- all container objects have the risk of overflowing the C stack if a
  long reference chain of them is created and then deallocated.  So, to
  be safe, the tp_dealloc methods for those objects should be protected
  from overflowing the stack.

- the Py_TRASHCAN_BEGIN and Py_TRASHCAN_END macros are hard to
  understand and a bit hard to use correctly.  Making the mechanism
  internal avoids confusion.  The code can be slightly simplified as
  well.

This proof-of-concept seems to pass tests but it will need some careful
review.  The exact rules related to calling GC Track/Untrack are subtle
and this changes things a bit.  I.e. tp_dealloc is called with GC
objects already untracked.  For 3rd party extensions, they are calling
PyObject_GC_UnTrack() and so I believe they should still work.

The fact that PyObject_CallFinalizerFromDealloc() wants GC objects to
definitely be tracked is a bit of a mystery to me (there is an assert to
check that).  I changed the code to track objects if they are not
already tracked but I'm not sure that's correct.

There could be a performance hit, due to the _PyType_IS_GC() test that
was added to the _Py_Dealloc() function.  For non-GC objects, that's
going to be a new branch and I'm worried it might hurt a bit.  OTOH,
maybe it's just in the noise.  Profiling will need to be done.
@nascheme nascheme added the DO-NOT-MERGE label Aug 12, 2021
@nascheme
nascheme requested review from 1st1, markshannon, methane and rhettinger as code owners August 12, 2021 06:35
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Aug 12, 2021
@bedevere-bot bedevere-bot added the awaiting core review label Aug 12, 2021
@nascheme
Remove unneeded GC TRACK/UNTRACK calls.
a069810
Since _Py_Dealloc will untrack, don't need to do it in tp_dealloc.  If
an object is resurrected, PyObject_CallFinalizerFromDealloc() will do
the track call.
@nascheme
nascheme requested review from ctismer and tim-one August 13, 2021 07:08
@nascheme
Make _Py_Dealloc use _PyObject_IS_GC().
692191f
I'm not sure _PyType_IS_GC() is safe in case the object returns false
from tp_is_gc().
@ambv ambv mentioned this pull request Aug 16, 2021
bpo-44881: Integrate GC untrack into trashcan begin. #27718
Closed
nascheme added 2 commits August 16, 2021 10:52
@nascheme
Small optimization for _Py_Dealloc().
6ce59ff
@nascheme
Tidy up code, improve comments.
92653a8
@github-actions

github-actions Bot commented Sep 16, 2021

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot 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:30
@ezio-melotti ezio-melotti removed the CLA signed label Jul 13, 2022
@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Aug 11, 2022
@markshannon markshannon mentioned this pull request Oct 14, 2022
More efficient deallocation. faster-cpython/ideas#482
Open
@vstinner vstinner mentioned this pull request Nov 9, 2022
gh-98724: Fix Py_CLEAR() macro side effects #99100
Merged
@nascheme

nascheme commented Apr 5, 2023

Copy link
Copy Markdown
Member Author

FYI, I believe this would have fixed GH-102356 and probably a lot of similar crashing bugs that have not yet been found. I would imagine most "container" (GC tracked) objects can be used to blow up the C stack on dealloc.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka
serhiy-storchaka marked this pull request as draft January 17, 2024 20:08
@bedevere-app bedevere-app Bot removed the awaiting core review label Jan 17, 2024
@nascheme

nascheme commented Sep 27, 2024

Copy link
Copy Markdown
Member Author

Since this PR is so out of date, I've created a new one: GH-124716

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nascheme nascheme closed this Sep 27, 2024
@nascheme nascheme mentioned this pull request Sep 27, 2024
Integrate trashcan mechanism into _Py_Dealloc #89060
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@methane methane Awaiting requested review from methane methane is a code owner
@tim-one tim-one Awaiting requested review from tim-one
@ctismer ctismer Awaiting requested review from ctismer
@cmaloney cmaloney Awaiting requested review from cmaloney cmaloney will be requested when the pull request is marked ready for review cmaloney is a code owner
@iritkatriel iritkatriel Awaiting requested review from iritkatriel iritkatriel will be requested when the pull request is marked ready for review iritkatriel is a code owner
@JelleZijlstra JelleZijlstra Awaiting requested review from JelleZijlstra JelleZijlstra will be requested when the pull request is marked ready for review JelleZijlstra is a code owner
@rhettinger rhettinger Awaiting requested review from rhettinger rhettinger will be requested when the pull request is marked ready for review rhettinger is a code owner
@ZeroIntensity ZeroIntensity Awaiting requested review from ZeroIntensity ZeroIntensity will be requested when the pull request is marked ready for review ZeroIntensity is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

DO-NOT-MERGE

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@nascheme @the-knights-who-say-ni @ezio-melotti @bedevere-bot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.