Repository navigation
gh-113141: Upgrade mimalloc to upstream v3 - #159718
DinoV wants to merge 39 commits into
Conversation
Sorry, something went wrong.
|
The following commit authors need to sign the Contributor License Agreement: |
Sorry, something went wrong.
Documentation build overview56 files changed · + 1 added · ± 55 modified + Added ± Modified |
Sorry, something went wrong.
Ah yes, but not really for the current PR as that includes the full mimalloc v3 vendored sources and that is a separate project under the MIT license. I can of course sign off for the changes I made on the CPython side. The current PR contains both, maybe we can split it? I think commit cedd47b is the CPython part only, while the other commits are just importing mimalloc v3 as a vendored library. |
Sorry, something went wrong.
daanx
left a comment
There was a problem hiding this comment.
Using MI_WIN_INIT_USE_FLS is not really encouraged for mimalloc. If we can use the default that would be best.
Sorry, something went wrong.
There's a crash on the Windows builds w/o it currently (and it seems to show up basically at startup but the freeze programs make it through it). I was planning on digging into it more later but this is what Claude came up as a fix - I just don't have immediate access to a Windows machine to see where the actual crash is coming from. But that's why it's there for now. Right now I'm just trying to get things to green CI and then I'll clean up the final result. |
Sorry, something went wrong.
That was still a 6% regression on memory (but things are headed in the right direction, THP tweak about it from 2x to 1.15, and then the _Py_qsbr_poll fix got us here), here's another run https://github.com/facebookexperimental/free-threading-benchmarking/actions/runs/39476892845 which has commit 3c8be4f that sets mi_option_purge_delay and mi_option_arena_purge_mult to their v2 values. |
Sorry, something went wrong.
|
Down to a 4% regression now: https://github.com/facebookexperimental/free-threading-benchmarking/tree/main/results/bm-20261020-3.16.0a1+dev-3c8be4f-NOGIL |
Sorry, something went wrong.
|
Another run with new options for mi_option_arena_purge_min_size, and mi_option_page_full_retain and a tweaked mi_option_arena_purge_mult: https://github.com/facebookexperimental/free-threading-benchmarking/actions/runs/39497633255 |
Sorry, something went wrong.
|
Down to 3% |
Sorry, something went wrong.
|
This is now neutral: https://github.com/facebookexperimental/free-threading-benchmarking/tree/main/results/bm-20261020-3.16.0a1+dev-a36efb6-NOGIL It includes one more upstream fix which is a36efb6 that re-enables the mi_arenas_try_purge call which tries to purge expired decommits. This was commented out but not removed in the 3x branch. @daanx Thanks for merging all of the other upstream PRs I sent. I can send another one to re-enable this but I can also throw it under another ifdef if you're concerned about having it on in the common case. But I think this basically brings us to parity where we could merge this. I'd still want to bring back the latest commits you have upstream but my previous run had that as neutral. I also am assuming your dev3 branch isn't yet released? |
Sorry, something went wrong.
Sync Objects/mimalloc and Include/internal/mimalloc with microsoft/mimalloc dev3 (1ff2382, "fix profile timestamps"). Most of the local edits previously carried in the vendored copy have been merged upstream (forceinline declaration order, WASI page size, quoted includes, MSan syscalls, Emscripten builtin allocator, MI_NO_ALIGN_FALLBACK_WARNING, MI_NO_CXX_NEW_HANDLER, __ImageBase, trailing whitespace). The FLS init assertion fix is dropped as CPython no longer uses MI_WIN_INIT_USE_FLS. Changes still carried on top of dev3 (proposed upstream): - Fix build with MI_NO_THP after the MADV_NOHUGEPAGE change - Fix -Wsign-compare warning when reading hpage_pmd_size - Fix data race on the thread-done pthread key at process exit CPython-specific change: - Purge expired ranges on arena free, as in mimalloc v2, to keep the peak memory of short-running processes at v2 levels. Define MI_NO_ALIGN_FALLBACK_WARNING in pycore_mimalloc.h, since the vendored copy now has the upstream opt-out instead of the warning being commented out.
|
Another benchmark run w/ the latest and greatest mimalloc/dev3 branch merged in: https://github.com/facebookexperimental/free-threading-benchmarking/actions/runs/39651451425 We're now down to 4 vendored in changes which I've opened upstream PRs for (the last being the most questionable I think): |
Sorry, something went wrong.
Sorry, something went wrong.
|
🤖 New build scheduled with the buildbot fleet by @DinoV for commit 7c4bdf4 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F159718%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
Sorry, something went wrong.
|
Still memory neutral after merging in the latest updates: https://github.com/facebookexperimental/free-threading-benchmarking/tree/main/results/bm-20261021-3.16.0a1+dev-7c4bdf4-NOGIL |
Sorry, something went wrong.
|
NoGIL refleaks pass: |
Sorry, something went wrong.
|
Latest perf run w/ zero upstream mimalloc changes and latest merge: https://github.com/facebookexperimental/free-threading-benchmarking/actions/runs/40000440133 |
Sorry, something went wrong.
This upgrades mimalloc to v3 - the initial work here was done by @daanx and this pushes it over the edge to getting all CI passing and conforming we're at parity with the existing implementation. v3 brings with it baked in qsbr support and it has a number of the CPython specific changes upstreamed now.
To maintain parity with v2 on the pyperformance benchmarks we set several options back to their v2 levels in pycore_mimalloc.h.