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 — gh-112529: Remove PyGC_Head from object pre-header in free-threaded build by colesbury · Pull Request #114564 · python/cpython

Conversation

@colesbury

@colesbury colesbury commented Jan 25, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

This avoids allocating space for PyGC_Head in the free-threaded build. The GC implementation for free-threaded CPython does not use the PyGC_Head structure.

  • The trashcan mechanism uses the ob_tid field instead of _gc_prev in the free-threaded build.
  • The GDB libpython.py file now determines the offset of the managed dict field based on whether the running process is a free-threaded build. Those are identified by the ob_ref_local field in PyObject.
  • Fixes _PySys_GetSizeOf(), which incorrectly incorrectly included the size of PyGC_Head in the size of static PyTypeObject.
  • Issue: Make the garbage collector thread-safe in --disable-gil builds #112529

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@colesbury
pythongh-112529: Remove PyGC_Head from object pre-header in free-thre…
7b60d93 
…aded build

This avoids allocating space for PyGC_Head in the free-threaded build.
The GC implementation for free-threaded CPython does not use the
PyGC_Head structure.

 * The trashcan mechanism uses the `ob_tid` field instead of `_gc_prev`
   in the free-threaded build.
 * The GDB libpython.py file now determines the offset of the managed
   dict field based on whether the running process is a free-threaded
   build. Those are identified by the `ob_ref_local` field in PyObject.
 * Fixes `_PySys_GetSizeOf()` which incorrectly incorrectly included the
   size of `PyGC_Head` in the size of static `PyTypeObject`.
@colesbury
colesbury requested review from DinoV, nascheme and pablogsal January 25, 2024 18:45
@bedevere-app bedevere-app Bot added the awaiting review label Jan 25, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Jan 25, 2024
Make the garbage collector thread-safe in --disable-gil builds #112529
Closed
3 tasks
@colesbury
Add blurb
87eb2fd
colesbury
colesbury commented Jan 25, 2024
View reviewed changes
Comment thread Python/sysmodule.c
Comment on lines +1882 to +1887
if (!Py_IS_TYPE(o, &PyType_Type) ||
PyType_HasFeature((PyTypeObject *)o, Py_TPFLAGS_HEAPTYPE))
{
/* Add the size of the pre-header if "o" is not a static type */
presize = _PyType_PreHeaderSize(Py_TYPE(o));
}

@colesbury colesbury Jan 25, 2024

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This now more closely matches the behavior of the Python check_sizeof function, although that function does not account for space for managed dict/weakref.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nascheme

nascheme commented Jan 26, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

This looks good to me.

I think the comment beside ob_tid should also note that it's used for the trashcan list.

It would be nice to have a bit more explanation of how ob_tid is being re-used by the GC. It wasn't totally clear to me when I was reviewing the previous PR. Maybe a comment somewhere inside gc_free_threading.c? My current understanding: when it is being used for the "gc refs" count, it cannot be used by the biased refcnt logic. So the GC code needs to be careful not to call any API that will fiddle refcnts. When it is being used as a list pointer (i.e. object stack), it does work as a valid thread-id and calling those APIs is safe (e.g. running finalizers).

Minor nit not really worth fixing: in libpython.py, I think the test could be if "ob_ref_local" in pyobj.fields()
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
nascheme
nascheme approved these changes Jan 26, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Jan 26, 2024
@nascheme

nascheme commented Jan 26, 2024

Copy link
Copy Markdown
Member

Another change that is now possible but maybe should be separate from this PR: _PyObject_IS_GC() could be optimized in the nogil build. That function is a bit expensive since it has to check for tp_is_gc() and possibly call it. Now that all objects have ob_gc_bits, we could store the "IS GC" status as another bit there, when the object is allocated.

In gc_free_threading.c, I think you mostly don't care about the "IS GC" check. You care if the tracked or finalized bits are set. You can just check those bits directly.

There are a couple potentially hot paths outside of gc_free_threading that call _PyObject_IS_GC() and they could just check a ob_gc_bits bit. It might be worth it. In my gh-44897 PR, having that bit would make it a bit faster. I still think enabling the "trashcan" for all GC objects is the right thing to do, rather than requiring the type of use the trashcan macros.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colesbury
Add additional comments based on code review.
6fa0a4c
@colesbury
colesbury requested a review from ericsnowcurrently as a code owner January 29, 2024 20:16
@colesbury

colesbury commented Jan 29, 2024

Copy link
Copy Markdown
Contributor Author

I think the comment beside ob_tid should also note that it's used for the trashcan list.

Added

My current understanding: when it is being used for the "gc refs" count, it cannot be used by the biased refcnt logic. So the GC code needs to be careful not to call any API that will fiddle refcnts. When it is being used as a list pointer (i.e. object stack), it does work as a valid thread-id and calling those APIs is safe (e.g. running finalizers).

Yeah, that's correct. I've added a comment in update_refs.

Minor nit not really worth fixing: in libpython.py, I think the test could be if "ob_ref_local" in pyobj.fields()

That doesn't work because pyobj.fields() returns a list of gdb.Field instances.

Another change that is now possible but maybe should be separate from this PR: _PyObject_IS_GC() could be optimized in the nogil build...

Good point. I'll do that in a follow up PR.
nascheme reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colesbury colesbury mentioned this pull request Jan 29, 2024
Optimize _PyObject_IS_GC in free-threaded build #114733
Closed
DinoV
DinoV approved these changes Feb 1, 2024
View reviewed changes
@DinoV
DinoV merged commit 587d480 into python:main Feb 1, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 1, 2024
@colesbury
colesbury deleted the gh-112529-gchead branch February 1, 2024 20:33
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@colesbury @aisk
pythongh-112529: Remove PyGC_Head from object pre-header in free-thre…
7dbca1b 
…aded build (python#114564)

* pythongh-112529: Remove PyGC_Head from object pre-header in free-threaded build

This avoids allocating space for PyGC_Head in the free-threaded build.
The GC implementation for free-threaded CPython does not use the
PyGC_Head structure.

 * The trashcan mechanism uses the `ob_tid` field instead of `_gc_prev`
   in the free-threaded build.
 * The GDB libpython.py file now determines the offset of the managed
   dict field based on whether the running process is a free-threaded
   build. Those are identified by the `ob_ref_local` field in PyObject.
 * Fixes `_PySys_GetSizeOf()` which incorrectly incorrectly included the
   size of `PyGC_Head` in the size of static `PyTypeObject`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DinoV DinoV DinoV approved these changes
@nascheme nascheme nascheme approved these changes
@pablogsal pablogsal Awaiting requested review from pablogsal
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

3 participants

@colesbury @nascheme @DinoV
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 01402520 — 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.