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

bpo-33597: Reduce PyGC_Head size #7043

Merged
merged 19 commits into from Jul 10, 2018

Conversation

Projects
None yet
6 participants
@methane
Member

methane commented May 22, 2018

g->gc.gc_prev = _PyGC_generation0->gc.gc_prev; \
g->gc.gc_prev->gc.gc_next = g; \
_PyGC_generation0->gc.gc_prev = g; \
_PyGCHead_SET_PREV(g, _PyGC_generation0->gc.gc_prev); \

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka May 22, 2018

Member

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

This comment has been minimized.

@methane

methane May 22, 2018

Member

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

g->gc.gc_next->gc.gc_prev = g->gc.gc_prev; \
assert(g->gc.gc_next != NULL); \
_PyGCHead_PREV(g)->gc.gc_next = g->gc.gc_next; \
_PyGCHead_SET_PREV(g->gc.gc_next, _PyGCHead_PREV(g)); \

This comment has been minimized.

@serhiy-storchaka

serhiy-storchaka May 22, 2018

Member

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

next = gc->gc.gc_next;
if (PyTuple_CheckExact(op)) {
_PyTuple_MaybeUntrack(op);
}

This comment has been minimized.

@methane

methane May 22, 2018

Member

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.

This comment has been minimized.

@pitrou

pitrou Jun 2, 2018

Member

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

@@ -227,20 +267,38 @@ append_objects(PyObject *py_list, PyGC_Head *gc_list)
return 0;
}
#if GC_DEBUG
static void
validate_list(PyGC_Head *head, uintptr_t expected_mask)

This comment has been minimized.

@methane

methane May 30, 2018

Member

@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.

This comment has been minimized.

@pitrou

pitrou Jun 2, 2018

Member

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

This comment has been minimized.

@methane

methane Jun 2, 2018

Member

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

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

This comment has been minimized.

@pitrou

pitrou Jun 2, 2018

Member

& 6? Why?

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; \

This comment has been minimized.

@pitrou

pitrou Jun 2, 2018

Member

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

This comment has been minimized.

@methane

methane Jun 2, 2018

Member

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

g->gc.gc_next = NULL; \
g->gc.gc_prev &= _PyGC_PREV_MASK_FINALIZED; \

This comment has been minimized.

@pitrou

pitrou Jun 2, 2018

Member

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

This comment has been minimized.

@methane

methane Jun 2, 2018

Member

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

This comment has been minimized.

@vstinner

vstinner Jun 22, 2018

Member

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

This comment has been minimized.

@methane

methane Jun 22, 2018

Member

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.
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;

This comment has been minimized.

@pitrou

pitrou Jun 2, 2018

Member

Can you add a small comment here explaining this field?

At the start of a collection, update_refs() copies the true refcount
to gc_refs, for each object in the generation being collected.
subtract_refs() then adjusts gc_refs so that it equals the number of
times an object is referenced directly from outside the generation
being collected.
gc_refs remains >= 0 throughout these steps.

This comment has been minimized.

@pitrou

pitrou Jun 2, 2018

Member

Why did you remove this line? Is it not true anymore?

This comment has been minimized.

@methane

methane Jun 2, 2018

Member

Pleviously, negative values are used for:

#define _PyGC_REFS_UNTRACKED                    (-2)
#define _PyGC_REFS_REACHABLE                    (-3)
#define _PyGC_REFS_TENTATIVELY_UNREACHABLE      (-4)

This pull request stop using these values. So gc_refs are always >=0.

static inline void
gc_set_prev(PyGC_Head *g, PyGC_Head *v)
{
g->gc.gc_prev = (g->gc.gc_prev & ~_PyGC_PREV_MASK)

This comment has been minimized.

@pitrou

pitrou Jun 2, 2018

Member

This is the same as _PyGCHead_SET_PREV(), right?

This comment has been minimized.

@methane

methane Jun 2, 2018

Member

There is small difference; _PyGCHead_SET_PREV(g, v) assumes v is pure pointer (no masks).
But only caller of this function passes pure pointer. So _PyGCHead_SET_PREV() can be used instead.
I'll remove this function.

@vstinner

Ok, here is a real review. When I wrote my first LGTM, I didn't read carefully the code, but the code also changed a lot since I reviewed it.

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.

This comment has been minimized.

@vstinner

vstinner Jun 22, 2018

Member

When I read gc_prev, I expect it to only contain a pointer to the previous entry, but it contains two information. Would it be possible to find a better name? (gc_prev might be fine) Maybe "gc_prev_flags"?

Maybe add "_" prefix to "explain" that the field must be accessed through macros? (ex: _gc_prev_flags)

If you rename the field, it makes sure that applications accessing (read or... write!) directly gc_prev will break which is a good thing IMHO.

Can you also please document which flags are stored in this field? Finalized, collecting and tentatively unreachable. Mention maybe that the two last are only used internally and so are not accessible?

This comment has been minimized.

@methane

methane Jun 22, 2018

Member

Can you also please document which flags are stored in this field? Finalized, collecting and tentatively unreachable. Mention maybe that the two last are only used internally and so are not accessible?

Finalized is documented below. And I don't want document other two bits in this file
because they are tightly coupled with GC algorithm. I added document in gcmodule.c instead.

This comment has been minimized.

@methane

methane Jun 28, 2018

Member

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

#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) \

This comment has been minimized.

@vstinner

vstinner Jun 22, 2018

Member

nitpick: you may take this change as an opportunity to add { ... } (PEP 7) :-)

This comment has been minimized.

@methane
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.
// Lowest three bits are used for flags.
} gc;
double dummy; /* force worst-case alignment */

This comment has been minimized.

@vstinner

vstinner Jun 22, 2018

Member

Hum, I think that alignment using sizeof(double) comes the C standard. If I'm right, it might help to document it here :-)

This comment has been minimized.

@methane

This comment has been minimized.

@methane

methane Jun 22, 2018

Member

Uh, it's not separated issue. Since I this pull request removes last Py_ssize_t member from gc, there are only two pointers (or integer having same size to pointer) in the struct.
It must be aligned 8byte on all 32bit and 64bit platforms. dummy can be removed!

This comment has been minimized.

@methane

methane Jun 28, 2018

Member

I removed dummy, and stop using union too.

#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)); \

This comment has been minimized.

@vstinner

vstinner Jun 22, 2018

Member

Maybe add a macro for (g->gc.gc_prev & _PyGC_PREV_MASK_INTERNAL)? Maybe #define _PyGCHead_REFS() or #define _PyGCHead_FLAGS()?

This comment has been minimized.

@methane

methane Jun 28, 2018

Member

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

g->gc.gc_next = NULL; \
g->gc.gc_prev &= _PyGC_PREV_MASK_FINALIZED; \

This comment has been minimized.

@vstinner

vstinner Jun 22, 2018

Member

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

}
static inline void
gc_clear_masks(PyGC_Head *g)

This comment has been minimized.

@vstinner

vstinner Jun 22, 2018

Member

You don't clear a mask but flags, no? Maybe: gc_clear_internal_flags()?

Maybe rename "internal flags" to "collection flags"? I don't know.

This comment has been minimized.

@methane

methane Jun 28, 2018

Member

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

/*** list functions ***/
static void
gc_list_init(PyGC_Head *list)
{
list->gc.gc_prev = list;
list->gc.gc_prev = (uintptr_t)list;

This comment has been minimized.

@vstinner

vstinner Jun 22, 2018

Member

I would prefer to use a macro here since the field is special. _PyGCHead_SET_PREV_FLAGS(list, 0)?

Ditto below.

This comment has been minimized.

@methane

methane Jun 22, 2018

Member

This code doesn't mean "clear flags". List header node (gc_list) doesn't have any flags.
So gc_prev is really pure pointer without any flags here.
I'll add one line comment here too.

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

This comment has been minimized.

@vstinner

vstinner Jun 22, 2018

Member

"v" should be "refs" no?

This comment has been minimized.

@methane

methane Jun 28, 2018

Member

fixed.

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

This comment has been minimized.

@vstinner

vstinner Jun 22, 2018

Member

Rename "v" to "refs"?

This comment has been minimized.

@methane

methane Jun 28, 2018

Member

fixed.

|| gc_refs == GC_UNTRACKED);
}
else {
assert(gc_refs > 0);

This comment has been minimized.

@vstinner

vstinner Jun 22, 2018

Member

Why flags are no longer tested?

This comment has been minimized.

@methane

methane Jun 22, 2018

Member

Because there are no GC_REACHABLE and GC_UNTRACKED now.
Instead, there are "early return" above.

+        if (gc->gc.gc_next == NULL || !gc_is_collecting(gc)) {
+            return 0

gc_refs == GC_REACHABLE meant the object is tracked but not in current gc_list. It is !gc_is_collecting(gc) now.

gc_refs == GC_UNTRACKED meant the object is not tracked. It is gc->gc.gc_next == NULL now.

@bedevere-bot

This comment has been minimized.

bedevere-bot commented Jun 22, 2018

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

@methane

This comment has been minimized.

Member

methane commented Jun 22, 2018

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

This comment has been minimized.

Member

pitrou commented Jun 22, 2018

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

This comment has been minimized.

Member

methane commented Jun 22, 2018

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 methane closed this Jun 22, 2018

@methane methane reopened this Jun 22, 2018

@methane

This comment has been minimized.

Member

methane commented Jun 28, 2018

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.

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.

This comment has been minimized.

@methane

methane Jun 28, 2018

Member

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

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.
// Lowest three bits are used for flags.
} gc;
double dummy; /* force worst-case alignment */

This comment has been minimized.

@methane

methane Jun 28, 2018

Member

I removed dummy, and stop using union too.

#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)); \

This comment has been minimized.

@methane

methane Jun 28, 2018

Member

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

#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) \

This comment has been minimized.

@methane
}
static inline void
gc_clear_masks(PyGC_Head *g)

This comment has been minimized.

@methane

methane Jun 28, 2018

Member

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

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

This comment has been minimized.

@methane

methane Jun 28, 2018

Member

fixed.

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

This comment has been minimized.

@methane

methane Jun 28, 2018

Member

fixed.

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

This comment has been minimized.

@methane

methane Jun 28, 2018

Member

added comment.

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

This comment has been minimized.

@methane

methane Jun 28, 2018

Member

added.

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

This comment has been minimized.

@methane

methane Jun 28, 2018

Member

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.

#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.

This comment has been minimized.

@pitrou

pitrou Jun 28, 2018

Member

Below? Do you mean above?

@@ -2121,8 +2122,7 @@ _PyTrash_destroy_chain(void)
PyObject *op = _PyRuntime.gc.trash_delete_later;
destructor dealloc = Py_TYPE(op)->tp_dealloc;
_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;

This comment has been minimized.

@pitrou

pitrou Jun 28, 2018

Member

Why not _PyGCHead_PREV(op)?

This comment has been minimized.

@methane

methane Jun 29, 2018

Member

fixed.

@@ -2159,8 +2159,7 @@ _PyTrash_thread_destroy_chain(void)
PyObject *op = tstate->trash_delete_later;
destructor dealloc = Py_TYPE(op)->tp_dealloc;
tstate->trash_delete_later =
(PyObject*) _Py_AS_GC(op)->gc.gc_prev;
tstate->trash_delete_later = (PyObject*) _Py_AS_GC(op)->_gc_prev;

This comment has been minimized.

@pitrou

pitrou Jun 28, 2018

Member

Same here.

This comment has been minimized.

@methane

methane Jun 29, 2018

Member

fixed.

#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

This comment has been minimized.

@pitrou

pitrou Jun 28, 2018

Member

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

This comment has been minimized.

@methane

methane Jun 29, 2018

Member

updated.

// 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.

This comment has been minimized.

@pitrou

pitrou Jun 28, 2018

Member

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

This comment has been minimized.

@methane

methane Jun 29, 2018

Member

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 added some commits Jun 28, 2018

@methane methane closed this Jul 6, 2018

@methane methane reopened this Jul 6, 2018

@methane

This comment has been minimized.

Member

methane commented Jul 7, 2018

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.

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

9 checks passed

VSTS: Linux-PR Linux-PR_20180706.22 succeeded
Details
VSTS: Linux-PR-Coverage Linux-PR-Coverage_20180706.24 succeeded
Details
VSTS: Windows-PR Windows-PR_20180706.22 succeeded
Details
VSTS: docs docs_20180706.30 succeeded
Details
VSTS: macOS-PR macOS-PR_20180706.22 succeeded
Details
bedevere/issue-number Issue number 33597 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@methane methane deleted the methane:two-word-gc branch Jul 18, 2018

lisroach added a commit to lisroach/cpython that referenced this pull request Sep 10, 2018

dacut added a commit to dacut/cpython that referenced this pull request Sep 16, 2018

yahya-abou-imran added a commit to yahya-abou-imran/cpython that referenced this pull request Nov 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment