gh-90815: Add mimalloc memory allocator #109914

DinoV · 2023-09-26T18:58:50Z

Issue: Add mimalloc memory allocator #90815

This adds mimalloc as an optional (but preferred when available) allocator to CPython. This is a bit of a mashup of the work from #109914 and the work of @colesbury to use mimalloc for no-gil and various updates to bring it up to current CPython.

The configuration logic added by @tiran is re-used and we keep pymalloc support unlike in the version from @colesbury. mimalloc is updated to 2.12 and along with a few changes @colesbury made to it.

This has run into some issues with subinterpreter support in that the allocator's are now stored in thread state and are per-thread. Sub interpreters in some scenarios will create a thread state on one thread and run that on another thread. Most of these are documented in the code base as being known issues. I've modified these so that we will find the right thread based upon the current thread ID and switch to it rather than getting the head thread. This seems pretty reasonable but looks pretty weird when we need to do it at interpreter shutdown.

ericsnowcurrently · 2023-09-27T16:19:33Z

FTR, @tiran's gh-31164 is a similar change and has a bunch of discussion.

CC @daanx

DinoV · 2023-09-27T17:59:12Z

@ericsnowcurrently Yep, and much of the autoconf stuff is taken from there... I just finally filled in the description with a little more background on where things are coming from :)

ericsnowcurrently · 2023-09-27T19:16:08Z

This has run into some issues with subinterpreter support in that the allocator's are now stored in thread state and are per-thread.

I expect that most of this PR is a relatively vanilla use of mimalloc in CPython, like @tiran's PR is. Would it be much trouble to keep the whole PR vanilla and have a follow-up PR that applies the customizations to mimalloc (e.g. per-thread allocator state)? That distinction would help, possibly a lot, when reviewing.

vstinner

For me, the change is mostly ok. But I would prefer that this PR doesn't switch the default Python memory allocator to mimalloc. Please write a separated PR for that. Also, I would prefer to move slowly, and only use mimalloc by default if Python is built with NoGIL.

Ah nice, the symbols are no longer exported:

$ ./configure --enable-shared --with-pydebug && make
$ readelf -s libpython3.13d.so.1.0 |grep mi_malloc$
 17640: 00000000001e2e31    37 FUNC    LOCAL  DEFAULT   12 mi_malloc

For comparison, PyLong_AsLong() is exported:

$ readelf -s libpython3.13d.so.1.0 |grep PyLong_AsLong$
  1694: 00000000001abc65    76 FUNC    GLOBAL DEFAULT   12 PyLong_AsLong
 18422: 00000000001abc65    76 FUNC    GLOBAL DEFAULT   12 PyLong_AsLong

Moreover, mimalloc/ headers moved to Include/internal/: good, good.

Support for mimalloc is always enabled: I think that it's a good think, that what I proposed before.

vstinner · 2023-10-21T09:02:03Z

Include/internal/pycore_pymem_init.h

+extern void _PyObject_MiFree(void *ctx, void *ptr);
+
+#  define PYMEM_ALLOC {NULL, _PyMem_MiMalloc, _PyMem_MiCalloc, _PyMem_MiRealloc, _PyMem_MiFree}
+#  define PYOBJ_ALLOC {NULL, _PyObject_MiMalloc, _PyObject_MiCalloc, _PyObject_MiRealloc, _PyObject_MiFree}


I would prefer to no use mimalloc by default for now. Not in this PR.

Maybe you can write a follow PR to use it by default if Python is built by NoGIL?

vstinner · 2023-10-21T09:13:26Z

Tools/build/smelly.py

+# mimalloc doesn't use static, but it's symbols are not exported
+# from the shared library.  They do show up in the static library
+# before its linked into an executable.
+ALLOWED_STATIC_PREFIXES = ('mi_', '_mi_')


Thank you for distinguishing local and exported symbols. It's good that "mi" and "mi" prefixes are only allowed for local symbols.

DinoV · 2023-10-23T22:22:49Z

Okay, the default is now pymalloc, for now PYTHONMALLOC=mimalloc ./python will pick up mimalloc. When we start to get nogil integration with it we'll need to make it the default/required in that config.

thesamesam · 2023-10-30T00:52:42Z

Is it possible for mimalloc?

That's not possible because we will have changes to mimalloc.

Is there any chance of those changes getting upstreamed? There's advantages for cpython then too -- way less work to rebase the internal copy and no real risk of bugs being cpython introduced.

Any thoughts? I think this got missed earlier in the review flurry. I've not seen any substantive discussion about why bundling is a good idea, which feels like it's worth proper consideration, given it's short-term easier but long-term expensive.

I understand upstreaming it initially is going to take some time, but it is going to be worthwhile for cpython upstream maintenance long-term too, as you can avoid:

having cpython being stuck on outdated versions in perpetuity, or
figuring out how to rebase (and include the local changes)
backporting fixes for various platforms/new compilers/etc to increasingly old versions of mimalloc

This also, of course, has the advantage of forcing one to document what the changes are which I think should be done either way.

Lib/test/pythoninfo.py

vstinner

See also issue #111499: PYTHONMALLOCSTATS=1 fails with fatal error at Python exit. Using mimalloc, Python is not affected :-)

mimalloc is not the default

vstinner · 2023-10-30T15:16:28Z

@DinoV: I pushed a few cleanup changes. I fixed the outdated doc which still said that mimalloc is the default.

vstinner · 2023-10-30T15:26:51Z

I cleaned the commit message to only keep most important information.

The configuration logic added by @tiran is re-used

I added @tiran as a co-author in the commit message.

ericsnowcurrently · 2023-10-30T15:40:55Z

Is there any chance of those changes getting upstreamed? There's advantages for cpython then too -- way less work to rebase the internal copy and no real risk of bugs being cpython introduced.

Any thoughts?

It's worth further discussion.

CC @daanx

vstinner · 2023-10-30T15:52:33Z

Congrats @DinoV, I merged your PR :-) Do you want to propose a follow-up PR to enable it by default when --disable-gil is used?

ericsnowcurrently · 2023-10-30T16:32:35Z

Looks like this broke the WASM buildbot: https://buildbot.python.org/all/#/builders/1046/builds/3371.

brettcannon · 2023-10-30T18:25:33Z

Looks like this broke the WASM buildbot: https://buildbot.python.org/all/#/builders/1046/builds/3371.

Yep, looks like there's an implicit function definition:

In file included from ../../Objects/obmalloc.c:15:
In file included from ../../Objects/mimalloc/static.c:37:
In file included from ../../Objects/mimalloc/prim/prim.c:19:
../../Objects/mimalloc/prim/wasi/prim.c:44:15: error: implicit declaration of function 'sbrk' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
    void* p = sbrk(size);
              ^
1 error generated.
make: *** [Makefile:2723: Objects/obmalloc.o] Error 1

vstinner · 2023-10-30T20:59:05Z

../../Objects/mimalloc/prim/wasi/prim.c:44:15: error: implicit declaration of function 'sbrk' is invalid in C99 [-Werror,-Wimplicit-function-declaration]

I wrote PR #111524 to fix WASI build.

vstinner · 2023-10-31T00:01:41Z

I wrote 4 follow-up PRs to fix different issues:

While mimalloc C code is built on Windows, Python cannot currently use mimalloc on Windows: see PR #111528.

* Add mimalloc v2.12 Modified src/alloc.c to remove include of alloc-override.c and not compile new handler. Did not include the following files: - include/mimalloc-new-delete.h - include/mimalloc-override.h - src/alloc-override-osx.c - src/alloc-override.c - src/static.c - src/region.c mimalloc is thread safe and shares a single heap across all runtimes, therefore finalization and getting global allocated blocks across all runtimes is different. * mimalloc: minimal changes for use in Python: - remove debug spam for freeing large allocations - use same bytes (0xDD) for freed allocations in CPython and mimalloc This is important for the test_capi debug memory tests * Don't export mimalloc symbol in libpython. * Enable mimalloc as Python allocator option. * Add mimalloc MIT license. * Log mimalloc in Lib/test/pythoninfo.py. * Document new mimalloc support. * Use macro defs for exports as done in: python#31164 Co-authored-by: Sam Gross <colesbury@gmail.com> Co-authored-by: Christian Heimes <christian@python.org> Co-authored-by: Victor Stinner <vstinner@python.org>

DinoV force-pushed the nogil/mimalloc_212 branch 7 times, most recently from 8b3ec52 to 06e86d7 Compare September 27, 2023 00:25

DinoV changed the title ~~Nogil/mimalloc 212~~ gh-90815: Add mimalloc memory allocator Sep 27, 2023

bedevere-app bot mentioned this pull request Sep 27, 2023

Add mimalloc memory allocator #90815

Closed

DinoV force-pushed the nogil/mimalloc_212 branch 4 times, most recently from e8c4f01 to 2dd8675 Compare September 27, 2023 15:59

DinoV requested a review from ericsnowcurrently September 27, 2023 16:04

DinoV force-pushed the nogil/mimalloc_212 branch from 2dd8675 to 672b6f4 Compare September 27, 2023 17:35

DinoV requested a review from colesbury September 27, 2023 17:36

DinoV force-pushed the nogil/mimalloc_212 branch from 672b6f4 to 41b62cb Compare September 27, 2023 17:52

DinoV force-pushed the nogil/mimalloc_212 branch from 41b62cb to 0dbe76b Compare September 27, 2023 18:19

DinoV force-pushed the nogil/mimalloc_212 branch 7 times, most recently from 7bda1e4 to 35d1ebd Compare September 28, 2023 19:52

DinoV marked this pull request as ready for review September 28, 2023 20:21

DinoV requested a review from rhettinger as a code owner September 28, 2023 20:21

vstinner reviewed Oct 21, 2023

View reviewed changes

Keep pymalloc the default

526a08d

vstinner reviewed Oct 30, 2023

View reviewed changes

Lib/test/pythoninfo.py Outdated Show resolved Hide resolved

vstinner added 2 commits October 30, 2023 15:40

Update Lib/test/pythoninfo.py

3d43b3a

license.rst: remove empty line at the end

3261bcb

vstinner reviewed Oct 30, 2023

View reviewed changes

vstinner added 4 commits October 30, 2023 16:13

Reduce Makefile.pre.in indentation

99c4a24

obmalloc.c: indent

e2e36dc

Split _PyObject_DebugMallocStats() in sub-functions

62b7b83

Fix memory doc

ab829b3

mimalloc is not the default

vstinner enabled auto-merge (squash) October 30, 2023 15:25

vstinner merged commit 05f2f0a into python:main Oct 30, 2023
29 checks passed

bedevere-app bot removed the awaiting review label Oct 30, 2023

erlend-aasland mentioned this pull request Oct 30, 2023

gh-90815: Add mimalloc memory allocator #31164

Closed

ayappanec mentioned this pull request Oct 31, 2023

AIX build breaks due to syscall.h not found #111544

Closed

oraluben mentioned this pull request Mar 19, 2024

Mimalloc header is not installed #116984

Closed

freakboy3742 mentioned this pull request Apr 18, 2024

Vendored mimalloc references macOS preprocessor symbols that no longer exist #118072

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-90815: Add mimalloc memory allocator #109914

gh-90815: Add mimalloc memory allocator #109914

DinoV commented Sep 26, 2023 •

edited

ericsnowcurrently commented Sep 27, 2023

DinoV commented Sep 27, 2023

ericsnowcurrently commented Sep 27, 2023

vstinner left a comment

vstinner Oct 21, 2023

vstinner Oct 21, 2023

DinoV commented Oct 23, 2023

thesamesam commented Oct 30, 2023 •

edited

vstinner left a comment

vstinner commented Oct 30, 2023

vstinner commented Oct 30, 2023

ericsnowcurrently commented Oct 30, 2023

vstinner commented Oct 30, 2023

ericsnowcurrently commented Oct 30, 2023

brettcannon commented Oct 30, 2023

vstinner commented Oct 30, 2023

vstinner commented Oct 31, 2023

gh-90815: Add mimalloc memory allocator #109914

gh-90815: Add mimalloc memory allocator #109914

Conversation

DinoV commented Sep 26, 2023 • edited

ericsnowcurrently commented Sep 27, 2023

DinoV commented Sep 27, 2023

ericsnowcurrently commented Sep 27, 2023

vstinner left a comment

Choose a reason for hiding this comment

vstinner Oct 21, 2023

Choose a reason for hiding this comment

vstinner Oct 21, 2023

Choose a reason for hiding this comment

DinoV commented Oct 23, 2023

thesamesam commented Oct 30, 2023 • edited

vstinner left a comment

Choose a reason for hiding this comment

vstinner commented Oct 30, 2023

vstinner commented Oct 30, 2023

ericsnowcurrently commented Oct 30, 2023

vstinner commented Oct 30, 2023

ericsnowcurrently commented Oct 30, 2023

brettcannon commented Oct 30, 2023

vstinner commented Oct 30, 2023

vstinner commented Oct 31, 2023

DinoV commented Sep 26, 2023 •

edited

thesamesam commented Oct 30, 2023 •

edited