Skip to content
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

Use mimalloc allocator for static build #7378

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

danleh
Copy link

@danleh danleh commented Mar 17, 2025

Which fixes performance problems, especially on heavily multi-threaded workloads (many functions, high core count machines), see #5561.

Which fixes performance problems, especially on heavily multi-threaded workloads (many functions, high core count machines), see WebAssembly#5561.
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! @kripken?

@tlively tlively requested a review from kripken March 17, 2025 21:55
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I worry about this kind of thing because the pinned version can get stale, but given the huge benefit here, it seems reasonable!

@dschuff
Copy link
Member

dschuff commented Mar 18, 2025

So I tried this out on my macbook and got some odd errors (they are different depending on whether mimalloc is linked as a static archive or an object library). Haven't figured out what's going on yet but we might want to gate this behind a CMake flag for now (otherwise it may break when we try to do an emscripten-releases release build).

@danleh
Copy link
Author

danleh commented Mar 18, 2025

So I tried this out on my macbook and got some odd errors (they are different depending on whether mimalloc is linked as a static archive or an object library). Haven't figured out what's going on yet but we might want to gate this behind a CMake flag for now (otherwise it may break when we try to do an emscripten-releases release build).

My bad. Added a MIMALLOC_STATIC option, configured the mimalloc build to only produce the static library, and enabled MIMALLOC_STATIC only in the Alpine Linux build (in both ci.yml and create_release.yml). Could you rerun CI?

@arsnyder16
Copy link
Contributor

looking forward to this being pull through into emsdk

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm but the CI error looks like it might be real?

@danleh
Copy link
Author

danleh commented Mar 18, 2025

https://github.com/WebAssembly/binaryen/actions/runs/13925295048/job/38975354336?pr=7378#step:10:2629

mimalloc: assertion failed: at "/src/third_party/mimalloc/src/segment.c":527, _mi_segments_collect
assertion: "tld->pages_purge.first == NULL"

Mh, it seems we are hitting microsoft/mimalloc#1031.

(@kripken Indeed. Seems we had a race with our comments.)

I'll try if a different commit/branch of mimalloc fixes the issue. Can reproduce locally (not sure why I didn't hit this yesterday locally. Maybe I only tested with LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libmimalloc.so.2 instead of static linking, not sure.)

which is the same version that Ubuntu 24.10 has in their mimalloc2.0 package, see https://launchpad.net/ubuntu/+source/mimalloc
@kripken
Copy link
Member

kripken commented Mar 18, 2025

Current error on CI looks like a warning from mimalloc. We don't want such warnings shown to our users - is there a way to disable them?

I must say, the warnings + that error do make me a little worried here. Is there perhaps a "stable" branch of mimalloc we should be using?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants