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.

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

gh-112529: Remove PyGC_Head from object pre-header in free-threaded build #114564

Merged
merged 3 commits into from
Feb 1, 2024

Conversation

colesbury
Copy link
Contributor

@colesbury colesbury commented Jan 25, 2024

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.

…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`.
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));
}
Copy link
Contributor Author

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.

@nascheme
Copy link
Member

nascheme commented Jan 26, 2024

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()

@nascheme
Copy link
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.

@colesbury
Copy link
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.

@DinoV DinoV merged commit 587d480 into python:main Feb 1, 2024
32 checks passed
@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
…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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants