Skip to content

Allow using mimalloc with dynamic linking #7391

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

Merged
merged 2 commits into from
Mar 29, 2025
Merged

Allow using mimalloc with dynamic linking #7391

merged 2 commits into from
Mar 29, 2025

Conversation

dschuff
Copy link
Member

@dschuff dschuff commented Mar 22, 2025

With dynamic linking, build and link mimalloc's dynamic library, and include it
in the installation (this also brings along the headers and CMake files, but it
seemed like more trouble than it was worth to try to manually install just the
library or remove the extras).
The static build remains the same.

Remove the restriction that mimalloc can only be linked into a static-lib build.
In dynamic mode, mimalloc gets linked into libbinaryen.so ...

With dynamic linking, build and link mimalloc's dynamic library, and include it
in the installation (this also brings along the headers and CMake files, but it
seemed like more trouble than it was worth to try to manually install just the
library or remove the extras).
The static build remains the same.

Remove the restriction that mimalloc can only be linked into a static-lib build.
In dynamic mode, mimalloc gets linked into libbinaryen.so ...
if(BUILD_STATIC_LIB)
target_link_libraries(binaryen mimalloc-static)
else()
target_link_libraries(binaryen mimalloc)
Copy link
Member

Choose a reason for hiding this comment

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

Don't you want the static build of mimalloc in both cases? i.e. aren't we trying to link mymalloc statically, even when using glibc / dynamic linking?

Copy link
Member Author

@dschuff dschuff Mar 24, 2025

Choose a reason for hiding this comment

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

That would be ideal but the problem is that libbinaryen is still a dynamic library. So the question would be, should we link mimalloc into libbinaryen.so or the tool? Linking it into libbinaryen.so would actually work today because none of the tools actually ever calls malloc (since the vast majority of the Binaryen code is in libbinaryen). But if we just landed it that way, if we ever did end up allocating in the tools, it would call system malloc. I think that would actually work too unless there were objects allocated on one side and freed on the other. So it seems like a risk of subtle bugs down the road, but maybe there's a way we could make any such failure obvious (maybe we'd already just have an obvious failure on free, I haven't actually tried it). We could also maybe statically link mimalloc into libbinaryen.so and re-export it (i.e. we'd export mi_malloc as malloc from libbinaryen.so). I don't know off the top of my head how to do this in ELF, but I bet it's possible.
But anyway, just building mimalloc as a DSO seemed simpler.

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking that libbinaryen.so would be left alone and that mimalloc.o (or mimalloc.a with --whole-archive) would get statically linking into each tool. I though that was how mimalloc recommended linking?

If dynamic linking works too that sounds reasonable to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case we'd need to export malloc from the tool and there would be a circular dependency between libbinaryen.so and the tool. Would that work in ELF if the DSO is loaded first?
I think the "preferred" method is either way

Copy link
Member

Choose a reason for hiding this comment

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

I was remembering this section: https://github.com/microsoft/mimalloc?tab=readme-ov-file#static-override

Here it sounds like malloc/free are exported by the main program and up overriding the malloc/free in glibc.. its it kind cyclic I suppose in that case? Anyway it doesn't sounds like that works on windows so maybe not a good approach anyway.

Copy link
Member

Choose a reason for hiding this comment

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

In that scenario malloc and free do need to be exported so that glibc.so can use them. glibc itself does call malloc and free and it would be very broken if it didn't use the mimalloc.o versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's ok for mimalloc to exist alongside the system allocator (e.g. they use separate mmap backing stores). This is why you can mix directly using mi_malloc/mi_free with the system allocator (https://github.com/microsoft/mimalloc?tab=readme-ov-file#using-the-library). The risk would be with freeing an object with the wrong allocator (e.g. allocation in strdup has to be freed with the system allocator).
In a non-LD_PRELOAD situation, would a malloc exported from the tool or from libbinaryen.so override the one in libc.so? That would be a circular dependency, not sure what would happen. I guess I can test that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually strdup gets redirected too, along with some others: https://microsoft.github.io/mimalloc/overrides.html I haven't yet been able to get it to fail with the dylib configuration.

Copy link
Member Author

@dschuff dschuff Mar 25, 2025

Choose a reason for hiding this comment

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

After looking at this a little more: In all the modes I tried on Linux (link mimalloc into its own .so, or into libbinaryen.so, or statically into libbinaryen.a), malloc always shows up as a global symbol in whatever it gets linked into, and it always seems to work). Basically all the combinations of static/dynamic libbinaryen and libmimalloc work. I guess there might be risks that one of these is more liketly to stop working or something but I'm not sure how much it matters.

I also did run some performance numbers against wasm-opt.wasm (so, something other than the GC benchmark in the other thread) on my cloudtop. It went about 28% faster (32s vs 45s) at a cost of about 10% more RSS. So it seems worth it.

I also tried on mac. Not all the combinations of static/dynamic linking work (got some errors from mixing heaps), but some do. But the speedup is almost negligible. I'm guessing because of the reduced parallelism compared to the cloudtop.

Choose a reason for hiding this comment

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

I was remembering this section: https://github.com/microsoft/mimalloc?tab=readme-ov-file#static-override

Here it sounds like malloc/free are exported by the main program and up overriding the malloc/free in glibc.. its it kind cyclic I suppose in that case? Anyway it doesn't sounds like that works on windows so maybe not a good approach anyway.

@dschuff
Copy link
Member Author

dschuff commented Mar 26, 2025

Updated the comments. I think malloc and free are exported from all the places they need to be and all the configurations seem to work. WDYT, should we do this?

# Do not show debug and warning messages of the allocator by default.
# (They can still be enabled via MIMALLOC_VERBOSE=1 wasm-opt ...)
add_compile_definitions(MI_DEBUG=0)

add_subdirectory(mimalloc EXCLUDE_FROM_ALL)
if(BUILD_STATIC_LIB)
# No need to install libmimalloc.a when it's linked statically into the tools.
Copy link
Member

Choose a reason for hiding this comment

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

Does BUILD_STATIC_LIB mean we are not also building as a shared libraries?

Copy link
Member Author

@dschuff dschuff Mar 26, 2025

Choose a reason for hiding this comment

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

Yes (see

binaryen/CMakeLists.txt

Lines 457 to 464 in fbf2010

if(BUILD_STATIC_LIB)
message(STATUS "Building libbinaryen as statically linked library.")
add_library(binaryen STATIC)
add_definitions(-DBUILD_STATIC_LIBRARY)
else()
message(STATUS "Building libbinaryen as shared library.")
add_library(binaryen SHARED)
endif()
)

@dschuff dschuff merged commit 1fd0085 into main Mar 29, 2025
14 checks passed
@dschuff dschuff deleted the mimalloc_dynamic branch March 29, 2025 01:01
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