Update mimalloc to 3.3.1#26696
Conversation
This reverts commit a4e07d4. In preparation for the next commit.
Resolves: emscripten-core#23090.
|
|
||
| This contains mimalloc 8c532c32c3c96e5ba1f2283e032f69ead8add00f (v2.1.7) with | ||
| This contains mimalloc 0ddf397796fbefa35b3278bd4431c2913a9892eb (v3.3.0) with | ||
| Emscripten-specific changes. |
There was a problem hiding this comment.
Are we tracking these emscripten-specific changes?
Was it easy enough for you to port them to the new version?
There was a problem hiding this comment.
Emscripten-specific changes are tracked in commit 8f4d480, which was created via:
- Replace
system/lib/mimalloc/with original mimalloc v2.1.7. - Commit those changes.
- Upgrade to mimalloc v3.3.0.
- Revert the changes from step 2.
There was a problem hiding this comment.
Nice. I wonder if its worth forking mimalloc in the emscripten-core org like we do for llvm and musl in order to keep track of these changes.
Perhaps not worth it as the changes are so small and we have the option to upstream most of them (I think?)
There was a problem hiding this comment.
It's probably not worth creating a fork for these small changes. The changes to src/prim/emscripten/prim.c could likely be upstreamed, but I'm less certain about those in src/alloc-override.c (although commit microsoft/mimalloc@a9b895e introduced a similar Emscripten-specific change to that file). I attempted to revert those weak decls with commit a4e07d4 but it was needed for test_wrap_malloc_mimalloc.
Commit 9e64b24 may also qualify as an Emscripten-specific change, but I'm not sure whether it should be documented (I've opened PR microsoft/mimalloc#1264 for this).
| @@ -8654,8 +8654,7 @@ def test_mallinfo(self): | |||
| @parameterized({ | |||
| '': ([],), | |||
| 'emmalloc': (['-sMALLOC=emmalloc'],), | |||
| # FIXME(https://github.com/emscripten-core/emscripten/issues/23090) | |||
| # 'mimalloc': (['-sMALLOC=mimalloc'],), | |||
| 'mimalloc': (['-sMALLOC=mimalloc', '-sABORTING_MALLOC=0'],), | |||
There was a problem hiding this comment.
Why is ABORTING_MALLOC=0 needed here bit not for the other variants of this test?
There was a problem hiding this comment.
It was somehow necessary for wasm64_4gb.test_wrap_malloc_mimalloc. mimalloc was likely over-allocating internally, and this change prevented an abort().
There was a problem hiding this comment.
Maybe worth a TODO to investigate?
| '-DMI_MAX_ALIGN_SIZE=8', | ||
| # reserve memory in 64 MiB chunks (internally divided by 4) | ||
| # Note: keep in sync with the -sINITIAL_HEAP default | ||
| '-DMI_DEFAULT_ARENA_RESERVE=65536', |
There was a problem hiding this comment.
It's in KiB.
emscripten/system/lib/mimalloc/src/options.c
Line 148 in 094d31e
The / 4 happens here:
emscripten/system/lib/mimalloc/src/arena.c
Lines 319 to 321 in 094d31e
There was a problem hiding this comment.
The size is indeed in Kb (confusing, but it is to avoid overflow on the long options for large reservations). So, it would reserve 64 MiB generally, but eventually actually reserve 16 MiB at a time since wasm doesn't have virtual reserve.
| # build mimalloc with an override of malloc/free | ||
| '-DMI_MALLOC_OVERRIDE', | ||
| # TODO: add build modes that include debug checks 1,2,3 | ||
| '-DMI_DEBUG=0', | ||
| # disable `assert()` in the underlying emmalloc allocator | ||
| '-DNDEBUG', | ||
| # avoid use of `__builtin_thread_pointer()` | ||
| '-DMI_LIBC_MUSL', |
There was a problem hiding this comment.
We are using musl so wouldn't we want to continue to define this?
There was a problem hiding this comment.
The MI_LIBC_MUSL definition is only used by:
emscripten/system/lib/mimalloc/include/mimalloc/prim.h
Lines 252 to 265 in 094d31e
Looking at commit microsoft/mimalloc@e46c114 and the linked issue, I don't think it applies to Emscripten.
There was a problem hiding this comment.
I will add a check for #ifndef MI_USE_BUILTIN_THREAD_POINTER -- that way you could keep -DMI_LIBC_MUSL and add -DMI_USE_BUILTIN_THREAD_POINTER=1 . Would that work for you?
Unfortunately, the builtin_thread_pointer support is hard to detect reliably so it is hard to avoid these kind of elaborate checks :-(
There was a problem hiding this comment.
Great! I re-added -DMI_LIBC_MUSL with commit 3841eab.
|
Thanks for working on this! Lgtm but let's wait for @kripken to chime in |
|
FYI: I just released v3.3.1 which has the fixes I suggested above. |
This reverts commit c3500f1.
To ensure the internal `_emscripten_yield()` function is called.
|
The latest revision of this PR updates mimalloc to v3.3.1. |
|
Lgtm. @kripken can you take a look? |
kripken
left a comment
There was a problem hiding this comment.
Great, thanks! Diff looks as expected.
|
I added |
sbc100
left a comment
There was a problem hiding this comment.
Ok, this should be good to land whenever you like @kleisauke.
(BTW we tend to use Fixes: #XX in our PR descriptions rather then Resolves:)
|
Thanks all! :) |
Fixes: #23090.
Supersedes: #23037.
Supersedes: #24542.