gh-112532: Use separate mimalloc heaps for GC objects #113263
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In `--disable-gil` builds, we now use four separate heaps in anticipation of using mimalloc to find GC objects when the GIL is disabled. To support this, we also make a few changes to mimalloc: * `mi_heap_t` and `mi_tld_t` initialization is split from allocation. This allows us to have a `mi_tld_t` per-`PyThreadState`, which is important to keep interpreter isolation, since the same OS thread may run in multiple interpreters (using different PyThreadStates.) * Heap abandoning (mi_heap_collect_ex) can now be called from a different thread than the one that created the heap. This is necessary because we may clear and delete the containing PyThreadStates from a different thread during finalization and after fork().
|
@DinoV - would you please review this? This is first PR in what will probably be a series of changes to get ready to use mimalloc from the GC. |
* The enum typedef will be convenient for future PRs that use the type. * Guarding the mimalloc includes allows us to unconditionally include pycore_mimalloc.h from other header files that rely on things like `struct _mimalloc_thread_state`.
DinoV
reviewed
Dec 20, 2023
| _mi_heap_init_ex(&mts->heaps[i], tld, _mi_arena_id_none()); | ||
| } | ||
|
|
||
| // By default, object allocations use _Py_MIMALLOC_HEAP_OBJECT. |
There was a problem hiding this comment.
I noticed this when looking at some of the later commits in your branch, but gc_alloc calls PyObject_Malloc. Is there a reason we can't add a PyGCObject_Malloc instead of doing this weird heap switching? It would presumably mean an expansion of the PyMemAllocatorDomain but it seems like that would be okay?
There was a problem hiding this comment.
@vstinner What do you think about adding a new domain for this vs having some oddness around allocating GC objects? We probably actually need 2 domains though for objects w/ and w/o pre-headers.
There was a problem hiding this comment.
Some context: all PyObject allocations go through PyObject_Malloc, but we need to use different mimalloc heaps for non-GC objects, GC objects, and GC objects with pre-headers. As written, we'll do this by modifying the mi_heap_t *current_object_heap field. But you could also do this by having a different memory domain for each heap. Something like PYMEM_DOMAIN_OBJ, PYMEM_DOMAIN_GC, PYMEM_DOMAIN_GC_PRE.
The advantage of adding new memory domains is that it is more explicit and cleaner.
The disadvantage is that code (including extensions) that uses PyMem_SetAllocator to intercept allocations will likely need to be modified to handle the new domains.
Include/internal/pycore_mimalloc.h
Outdated
| mi_heap_t heaps[_Py_MIMALLOC_HEAP_COUNT]; | ||
| mi_tld_t tld; | ||
| #else | ||
| char _unused; // empty structs are not allowed |
There was a problem hiding this comment.
It seems like this is only used when Py_GIL_DISABLED is defined, so maybe just not define the struct at all if the GIL is enabled?
DinoV
approved these changes
Dec 21, 2023
|
|
|
@colesbury It's related to this PR. |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In
--disable-gilbuilds, we now use four separate heaps in preparation for when we will use mimalloc to find GC objects when the GIL is disabled. This requires making a few changes to mimalloc:mi_heap_tandmi_tld_tinitialization is split from allocation. This allows us to have ami_tld_tper-PyThreadState, which is important to keep interpreter isolation, since the same OS thread may run in multiple interpreters (using different PyThreadStates.)Heap abandoning (
mi_heap_collect_ex) can now be called from a different thread than the one that created the heap. This is necessary because we may clear and delete the containing PyThreadStates from a different thread during finalization and after fork().--disable-gilbuilds #112532