bpo-33597: Reduce PyGC_Head size#7043

methane · 2018-05-22T07:07:22Z

https://bugs.python.org/issue33597

serhiy-storchaka · 2018-05-22T12:23:40Z

Include/objimpl.h

Seems _PyGC_generation0->gc.gc_prev can have the lowest bits set. This will trigger an assertion in _PyGCHead_SET_PREV().

Only Python objects use lower bits. Link header (PyGC_Head without Python object) must not have flags.
(Surely, I need to add more comments.)

serhiy-storchaka · 2018-05-22T12:24:45Z

Include/objimpl.h

_PyGCHead_PREV(g) is called twice. Wouldn't be better to cache the result?

methane · 2018-05-22T14:33:13Z

Modules/gcmodule.c

Since gc->gc.gc_next->gc.gc_prev is not real prev pointer, we can't call _PyTuple_MaybeUntrack(op) here.
I moved it to split function. But it means this pull request adds one more link traversal.

By the way, current code is not idiomatic. While most tuples can be untracked, there are still many tuples
which arn't be untracked. For example, __mro__ or __bases__.
In final generation, we check all tuples contents. In applications which triggers final generation GC frequently,
and there are many tuples, it can be significant overhead.

Anyway, I need to find "tracked tuple heavy" application to benchmark before optimize here.

If final generation GC is triggered frequently, it means our heuristic is inadequate. Full collections can be very expensive.

methane · 2018-05-30T04:58:12Z

Modules/gcmodule.c


+#if GC_DEBUG
+static void
+validate_list(PyGC_Head *head, uintptr_t expected_mask)


@pitrou @serhiy-storchaka
How do you think this function? Should I remove these checks and GC_DEBUG flag?

This validation was useful while hacking, and I think it's good document about when MASK flags are set and cleared.
But I can replace them to just a line of comment, of course.

I'm ok with this function. Have you tried to always enable it in debug mode, or is it too slow?

I enabled it while I'm debugging. It made test significantly slower.

vstinner

LGTM. The performance seems acceptable, and reducing the memory footprint can only be a good idea :-)

pitrou · 2018-06-02T08:05:42Z

Include/objimpl.h

+    if (g->gc.gc_next != NULL) \
        Py_FatalError("GC object already tracked"); \
-    _PyGCHead_SET_REFS(g, _PyGC_REFS_REACHABLE); \
+    assert((g->gc.gc_prev & 6) == 0); \


& 6? Why?

pitrou · 2018-06-02T08:07:10Z

Include/objimpl.h

-    g->gc.gc_next->gc.gc_prev = g->gc.gc_prev; \
+    PyGC_Head *prev = _PyGCHead_PREV(g); \
+    assert(g->gc.gc_next != NULL); \
+    prev->gc.gc_next = g->gc.gc_next; \


Hmm... I wonder if the look would less weird if we also had _PyGCHead_NEXT and _PyGCHead_SET_NEXT macros.

I skip using _PyGCHead_PREV and _PyGCHead_SET_PREV when flags must not be set. (i.e. list header) too.

pitrou · 2018-06-02T08:09:06Z

Include/objimpl.h

+    prev->gc.gc_next = g->gc.gc_next; \
+    _PyGCHead_SET_PREV(g->gc.gc_next, prev); \
    g->gc.gc_next = NULL; \
+    g->gc.gc_prev &= _PyGC_PREV_MASK_FINALIZED; \


Is that the same as _PyGCHead_SET_PREV(g, NULL)? The latter seems clearer to me.

Bit 1 must be kept after untracking.
But bit 2 and 3 must be cleared when untracking, because this macro may be called while GC
(i.e. weakref callback or tp_finalize).

Why not adding a macro setting the pointer and flags at once? _PyGCHead_SET_PREV_FLAGS(g->gc.gc_next, prev, flags).

Because it's not useful. gc_prev is used as:

Set prev pointer, without touching any flags. (_PyTrash_deposit_object and _PyObject_GC_TRACK)

Set prev pointer to list header node. Flags are not used in list header. (_PyObject_GC_TRACK)

Clear prev pointer and flags, but keep only _PyGC_PREV_MASK_FINALIZED. (only here.)

Clear gc_prev entirely. = (uintptr_t)0.

pitrou · 2018-06-02T08:10:38Z

Include/objimpl.h

-        union _gc_head *gc_prev;
-        Py_ssize_t gc_refs;
+        union _gc_head *gc_next;  // NULL means the object is not tracked
+        uintptr_t gc_prev;


Can you add a small comment here explaining this field?

bedevere-bot · 2018-06-22T10:58:14Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

methane · 2018-06-22T13:25:32Z

Apart from that, I have another worrying.
Old 32bit Windows had /3GB option which make user space 3GB instead of 2GB.
Since each pointer has 4bytes, upper bound of refcnt can be 3 * 2^28, which is larger than 2^29.
It means refcnt << 3 is not safe on such platform.

I found /LARGEADDRESSAWARE linker flag is required to use 3GB user space on Windows.
Since it's not used when building Python, it's safe to trim 3 bits from refcnt.

Now I think keeping two implementation is not worth enough.

pitrou · 2018-06-22T13:48:50Z

I found /LARGEADDRESSAWARE linker flag is required to use 3GB user space on Windows.
Since it's not used when building Python, it's safe to trim 3 bits from refcnt.

It's not only about Windows, other OSes can do it too (perhaps Linux does?).

methane · 2018-06-22T14:35:24Z

You're right... Linux can use 3GB for user virtual address. And it's default for x86.

Then, (a) Use int64_t instead of intpr_t, or (b) keep two implementation...

I think I should try (b).

methane · 2018-06-28T04:46:09Z

I found UNREACHABLE flag is only needed in move_unreachable() essentially.
So I removed the flag from gc_prev, and use gc_next for it temporary.

Now we require 4bytes aligned pointer, and support 1G-1 refcnt on 32bit (4bytes for each pointer) platform.

methane

PTAL

methane · 2018-06-28T07:55:35Z

Include/objimpl.h

-        union _gc_head *gc_prev;
-        Py_ssize_t gc_refs;
+        union _gc_head *gc_next;  // NULL means the object is not tracked
+        uintptr_t gc_prev;  // Pointer to previous object in the list.


I changed member name to _gc_prev and _gc_next.
I don't want 3rd party touches them directly.

methane · 2018-06-28T07:55:53Z

Include/objimpl.h

+        uintptr_t gc_prev;  // Pointer to previous object in the list.
+                            // Lowest three bits are used for flags.
    } gc;
    double dummy;  /* force worst-case alignment */


I removed dummy, and stop using union too.

methane · 2018-06-28T07:58:18Z

Include/objimpl.h

+#define _PyGCHead_SET_PREV(g, p) do { \
+    assert(((uintptr_t)p & ~_PyGC_PREV_MASK) == 0); \
+    (g)->gc.gc_prev = ((g)->gc.gc_prev & ~_PyGC_PREV_MASK) \
+        | ((uintptr_t)(p)); \


I don't think it's worth enough...

methane · 2018-06-28T07:59:20Z

Include/objimpl.h

 #define _PyObject_GC_TRACK(o) do { \
    PyGC_Head *g = _Py_AS_GC(o); \
-    if (_PyGCHead_REFS(g) != _PyGC_REFS_UNTRACKED) \
+    if (g->gc.gc_next != NULL) \


methane · 2018-06-28T08:00:00Z

Modules/gcmodule.c

+}
+
+static inline void
+gc_clear_masks(PyGC_Head *g)


Since I removed MASK_TENTATIVELY_UNREACHABLE from gc_prev, I renamed this to gc_clear_collecting.

methane · 2018-06-28T08:00:07Z

Modules/gcmodule.c

+}
+
+static inline void
+gc_set_refs(PyGC_Head *g, Py_ssize_t v)


methane · 2018-06-28T08:00:12Z

Modules/gcmodule.c

+}
+
+static inline void
+gc_reset_refs(PyGC_Head *g, Py_ssize_t v)


methane · 2018-06-28T08:01:00Z

Modules/gcmodule.c

+                    (visitproc)visit_reachable,
+                    (void *)young);
+            gc_set_prev(gc, prev);
+            gc->gc.gc_prev &= ~MASK_COLLECTING;


added comment.

methane · 2018-06-28T08:01:12Z

Modules/gcmodule.c

+        }
+        gc = next;
+    }
+    young->gc.gc_prev = (uintptr_t)prev;


methane · 2018-06-28T08:03:29Z

Modules/gcmodule.c

+        if (gc_get_refs(gc) != 0) {
+            ret = -1;
+        }
+        _PyGCHead_SET_PREV(gc, prev);


doubly-linked list is broken and restored in this function. So no need to add it in function description.
+ // Use gc_refs and break gc_prev again. comment is above in this function.

pitrou · 2018-06-28T16:41:17Z

Include/objimpl.h

+#define _PyGCHead_NEXT(g)        ((PyGC_Head*)(g)->_gc_next)
+#define _PyGCHead_SET_NEXT(g, p) ((g)->_gc_next = (uintptr_t)(p))
+
+// Lowest two bits of _gc_prev is used for flags described below.


Below? Do you mean above?

pitrou · 2018-06-28T16:42:35Z

Objects/object.c


-        _PyRuntime.gc.trash_delete_later =
-            (PyObject*) _Py_AS_GC(op)->gc.gc_prev;
+        _PyRuntime.gc.trash_delete_later = (PyObject*) _Py_AS_GC(op)->_gc_prev;


Why not _PyGCHead_PREV(op)?

pitrou · 2018-06-28T16:42:44Z

Objects/object.c


-        tstate->trash_delete_later =
-            (PyObject*) _Py_AS_GC(op)->gc.gc_prev;
+        tstate->trash_delete_later = (PyObject*) _Py_AS_GC(op)->_gc_prev;


pitrou · 2018-06-28T16:43:30Z

Modules/gcmodule.c

+#define GC_NEXT _PyGCHead_NEXT
+#define GC_PREV _PyGCHead_PREV
+
+// Bit 0 of _gc_prev is used for _PyGC_PREV_MASK_FINALIZED in objimpl.h


The comment doesn't seem to match the line being commented.

pitrou · 2018-06-28T16:44:20Z

Modules/gcmodule.c

+// move_legacy_finalizers() removes this flag instead.
+// Between them, unreachable list is not normal list and we can not use
+// most gc_list_* functions for it.  We should manually tweaking unreachable
+// list in these two functions.


I don't understand "We should manually tweaking unreachable list in these two functions".

Removed last sentence.

I meant we should manually set _gc_prev and _gc_next, like this:

+ // Manually unlink gc from unreachable list because + PyGC_Head *prev = GC_PREV(gc); + PyGC_Head *next = (PyGC_Head*)(gc->_gc_next & ~NEXT_MASK_UNREACHABLE); + assert(prev->_gc_next & NEXT_MASK_UNREACHABLE); + assert(next->_gc_next & NEXT_MASK_UNREACHABLE); + prev->_gc_next = gc->_gc_next; // copy NEXT_MASK_UNREACHABLE

methane · 2018-07-07T15:48:09Z

If there are no more objections, I want to merge this in next week.
We can revert it or add switch to use legacy GC later when we need.

the-knights-who-say-ni added the CLA signed label May 22, 2018

bedevere-bot added the awaiting merge label May 22, 2018

methane added the DO-NOT-MERGE label May 22, 2018

serhiy-storchaka self-requested a review May 22, 2018 10:31

serhiy-storchaka reviewed May 22, 2018

View reviewed changes

methane commented May 22, 2018

View reviewed changes

methane added 3 commits May 28, 2018 19:02

Reduce PyGC_Head size

161eee1

Update comments

561e2d5

Update _PyObject_GC_UNTRACK

184841a

methane force-pushed the two-word-gc branch from 9082f0f to 184841a Compare May 28, 2018 10:02

methane added 2 commits May 28, 2018 21:17

Add NEWS entry

16c66ca

update

cedc2e5

methane commented May 30, 2018

View reviewed changes

Simplify _PyGC_SET_FINALIZED

3b23da2

methane added performance Performance or resource usage awaiting review and removed DO-NOT-MERGE awaiting merge labels May 30, 2018

methane closed this May 30, 2018

methane reopened this May 30, 2018

methane closed this Jun 1, 2018

methane reopened this Jun 1, 2018

methane force-pushed the two-word-gc branch from 26171ce to 3b23da2 Compare June 1, 2018 07:09

vstinner approved these changes Jun 1, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jun 1, 2018

pitrou reviewed Jun 2, 2018

View reviewed changes

bedevere-bot added the awaiting changes label Jun 22, 2018

methane closed this Jun 22, 2018

methane reopened this Jun 22, 2018

Move clinic header

ac3d5a8

Use only lowest two bits of gc_prev.

f0c305c

methane force-pushed the two-word-gc branch from 3092c3e to f0c305c Compare June 28, 2018 06:21

methane commented Jun 28, 2018

View reviewed changes

pitrou reviewed Jun 28, 2018

View reviewed changes

methane and others added 2 commits June 29, 2018 02:02

fix comment

d7755a8

fixup

38c55a0

methane closed this Jul 6, 2018

methane reopened this Jul 6, 2018

methane merged commit 5ac9e6e into python:master Jul 10, 2018

bedevere-bot removed the awaiting changes label Jul 10, 2018

methane deleted the two-word-gc branch July 18, 2018 11:08

eduardo-elizondo mentioned this pull request Apr 11, 2020

gh-84436: Implement Immortal Objects #19474

Merged

This was referenced Aug 8, 2025

Fix obsolete comments in gc.c #137562

Open

gh-137562: Remove reference for GC_REACHABLE because it is obsolete #137563

Merged

Uh oh!

Conversation

methane commented May 22, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jun 22, 2018

Uh oh!

methane commented Jun 22, 2018

Uh oh!

pitrou commented Jun 22, 2018

Uh oh!

methane commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

methane commented Jun 28, 2018

Uh oh!

methane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

methane commented May 22, 2018 •

edited by bedevere-bot

Loading

methane commented Jun 22, 2018 •

edited

Loading