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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a840ae1
Allow using mimalloc with dynamic linking
dschuff Mar 21, 2025
a980ec2
Update comments
dschuff Mar 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ jobs:

- name: cmake
run: |
./alpine.sh cmake . -G Ninja -DCMAKE_INSTALL_PREFIX=out/install -DCMAKE_CXX_FLAGS="-static" -DCMAKE_C_FLAGS="-static" -DCMAKE_BUILD_TYPE=Release -DBUILD_STATIC_LIB=ON -DMIMALLOC_STATIC=ON -DCMAKE_INSTALL_PREFIX=install
./alpine.sh cmake . -G Ninja -DCMAKE_INSTALL_PREFIX=out/install -DCMAKE_CXX_FLAGS="-static" -DCMAKE_C_FLAGS="-static" -DCMAKE_BUILD_TYPE=Release -DBUILD_STATIC_LIB=ON -DBUILD_MIMALLOC=ON -DCMAKE_INSTALL_PREFIX=install

- name: build
run: |
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/create_release.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ jobs:

- name: cmake
run: |
./alpine.sh cmake . -G Ninja -DCMAKE_CXX_FLAGS="-static" -DCMAKE_C_FLAGS="-static" -DCMAKE_BUILD_TYPE=Release -DBUILD_STATIC_LIB=ON -DMIMALLOC_STATIC=ON -DCMAKE_INSTALL_PREFIX=install
./alpine.sh cmake . -G Ninja -DCMAKE_CXX_FLAGS="-static" -DCMAKE_C_FLAGS="-static" -DCMAKE_BUILD_TYPE=Release -DBUILD_STATIC_LIB=ON -DBUILD_MIMALLOC=ON -DCMAKE_INSTALL_PREFIX=install

- name: build
run: |
Expand Down
19 changes: 12 additions & 7 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,10 @@ endif()
# Advised to turn on when statically linking against musl libc (e.g., in the
# Alpine Linux build we use for producing official Linux binaries), because
# musl libc's allocator has very bad performance on heavily multi-threaded
# workloads / high core count machines.
# workloads / high core count machines. But it also works with dynamic linking
# with a small performance advantage in some cases over the glibc allocator.
# See https://github.com/WebAssembly/binaryen/issues/5561.
option(MIMALLOC_STATIC "Build with statically linked mimalloc allocator" OFF)
option(BUILD_MIMALLOC "Build with mimalloc allocator" OFF)

# Turn this off to install only tools and not static/dynamic libs.
option(INSTALL_LIBS "Install libraries" ON)
Expand Down Expand Up @@ -467,12 +468,16 @@ if(BUILD_LLVM_DWARF)
target_link_libraries(binaryen llvm_dwarf)
endif()

if(MIMALLOC_STATIC)
if(NOT(CMAKE_SYSTEM_NAME STREQUAL "Linux" AND BUILD_STATIC_LIB) OR EMSCRIPTEN)
message(FATAL_ERROR "Statically linking mimalloc is only supported when building as a native, statically linked library on Linux.")
if(BUILD_MIMALLOC)
if(NOT(CMAKE_SYSTEM_NAME STREQUAL "Linux") OR EMSCRIPTEN)
message(FATAL_ERROR "Linking mimalloc is only supported on Linux.")
endif()
message(STATUS "Building with mimalloc allocator.")
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 •
edited
Loading

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 •
edited
Loading

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.

Copy link

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.

endif()
message(STATUS "Building with statically linked mimalloc allocator.")
target_link_libraries(binaryen mimalloc-static)
endif()

add_subdirectory(src/ir)
Expand Down
18 changes: 13 additions & 5 deletions third_party/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,23 @@ if(BUILD_LLVM_DWARF)
add_subdirectory(llvm-project)
endif()

if(MIMALLOC_STATIC)
# We only need the static library, nothing else.
set(MI_BUILD_STATIC ON)
set(MI_BUILD_SHARED OFF)
if(BUILD_MIMALLOC)
# Match static/dynamic linking between libbinaryen and mimalloc
set(MI_BUILD_STATIC ${BUILD_STATIC_LIB})
if (BUILD_STATIC_LIB)
set(MI_BUILD_SHARED OFF)
endif()
set(MI_BUILD_OBJECT OFF)
set(MI_BUILD_TESTS OFF)
set(MI_INSTALL_TOPLEVEL ON)
# 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 •
edited
Loading

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()
)

add_subdirectory(mimalloc EXCLUDE_FROM_ALL)
else()
add_subdirectory(mimalloc)
endif()
endif()
Loading