Skip to content

mimalloc integration #2708

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

Open
Vansh-kap-98 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

mimalloc integration #2708

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
66cfe1d
perf: integrate mimalloc for core benchmarking
Vansh-kap-98 Aug 6, 2026
0af17b8
Fix mimalloc target naming, sanitizer ordering, and restore corrupted…
Vansh-kap-98 Aug 6, 2026
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
1 change: 1 addition & 0 deletions .gitattributes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
/vendor/** linguist-generated=true
vendor/*.mask text eol=lf
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,6 @@ _deps
Brewfile.lock.json
.DS_Store
.cache
out/
CMakeSettings.json

@jviotti jviotti Aug 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change

18 changes: 14 additions & 4 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.24)
project(core VERSION 0.0.0 LANGUAGES C CXX ASM_MASM DESCRIPTION "Sourcemeta Core")
list(APPEND CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/cmake")

# Options
#Options

@jviotti jviotti Aug 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
#Options
# Options

option(SOURCEMETA_CORE_LANG_PREPROCESSOR "Build the Sourcemeta Core language preprocessor library" ON)
option(SOURCEMETA_CORE_LANG_IO "Build the Sourcemeta Core language I/O library" ON)
option(SOURCEMETA_CORE_LANG_PROCESS "Build the Sourcemeta Core language Process library" ON)
Expand Down Expand Up @@ -55,19 +55,29 @@ option(SOURCEMETA_CORE_CONTRIB_GOOGLEBENCHMARK "Build the GoogleBenchmark librar

include(Sourcemeta)

# Don't force downstream consumers on this
sourcemeta_option_enum(

@jviotti jviotti Aug 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If minalloc is indeed consistently faster, then no need to have it as an option. Let's just compile it unconditionally?

NAME SOURCEMETA_CORE_ALLOCATOR
DEFAULT "system"
CHOICES "system" "mimalloc"
DESCRIPTION "Memory allocator to use: system or mimalloc")

#Don't force downstream consumers on this
if(PROJECT_IS_TOP_LEVEL)
sourcemeta_enable_simd()
endif()

# Enable the sanitizers before defining any target
#Enable the sanitizers before defining any target

@jviotti jviotti Aug 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like in general most comments lost the initial space for some reason?

@Vansh-kap-98 Vansh-kap-98 Aug 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yes i apologise for the missing spaces, while i was manually reviewing changes i instinctually removed the space out of habit. Im gonna review once again and revert these in the next commit.

if(SOURCEMETA_CORE_ADDRESS_SANITIZER)
sourcemeta_sanitizer(TYPE address)
elseif(SOURCEMETA_CORE_UNDEFINED_SANITIZER)
sourcemeta_sanitizer(TYPE undefined)
endif()

# TODO: Turn this into a re-usable utility CMake function
if(SOURCEMETA_CORE_ALLOCATOR STREQUAL "mimalloc")
find_package(Mimalloc REQUIRED)

@cubic-dev-ai cubic-dev-ai Bot Aug 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3: The new SOURCEMETA_CORE_ALLOCATOR option is declared at the top-level and immediately drives find_package(Mimalloc REQUIRED), but the only target that ever links the resulting library is the benchmark (and benchmarks default OFF). So a user who picks "mimalloc" expecting the core library stack to use it gets nothing changed, while still paying for a full mimalloc configure/build and risking a hard configure failure from the REQUIRED find when only the non-benchmark build is wanted. Consider gating the find_package (and ideally the option's effect) behind SOURCEMATA_CORE_BENCHMARK, and either documenting that the allocator only applies to the benchmark or actually threading the link into the core libraries so the option does what its description says.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At CMakeLists.txt, line 77:

<comment>The new SOURCEMETA_CORE_ALLOCATOR option is declared at the top-level and immediately drives `find_package(Mimalloc REQUIRED)`, but the only target that ever links the resulting library is the benchmark (and benchmarks default OFF). So a user who picks "mimalloc" expecting the core library stack to use it gets nothing changed, while still paying for a full mimalloc configure/build and risking a hard configure failure from the REQUIRED find when only the non-benchmark build is wanted. Consider gating the find_package (and ideally the option's effect) behind SOURCEMATA_CORE_BENCHMARK, and either documenting that the allocator only applies to the benchmark or actually threading the link into the core libraries so the option does what its description says.</comment>

<file context>
@@ -55,19 +55,29 @@ option(SOURCEMETA_CORE_CONTRIB_GOOGLEBENCHMARK "Build the GoogleBenchmark librar
 
-# TODO: Turn this into a re-usable utility CMake function
+if(SOURCEMETA_CORE_ALLOCATOR STREQUAL "mimalloc")
+  find_package(Mimalloc REQUIRED)
+endif()
+
</file context>

endif()

#TODO : Turn this into a re - usable utility CMake function
if(SOURCEMETA_CORE_INSTALL)
include(GNUInstallDirs)
include(CMakePackageConfigHelpers)
Expand Down
1 change: 1 addition & 0 deletions DEPENDENCIES
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
vendorpull https://github.com/sourcemeta/vendorpull 89f348a97842e05aeab45d338d41fb02031fad62
mimalloc https://github.com/microsoft/mimalloc v3.4.4
jsontestsuite https://github.com/nst/JSONTestSuite d64aefb55228d9584d3e5b2433f720ea8fd00c82
yaml-test-suite https://github.com/yaml/yaml-test-suite data-2022-01-17
cmark-gfm https://github.com/github/cmark-gfm 587a12bb54d95ac37241377e6ddc93ea0e45439b
Expand Down
4 changes: 4 additions & 0 deletions benchmark/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ if(BENCHMARK_SOURCES)
target_compile_definitions(sourcemeta_core_benchmark
PRIVATE CURRENT_DIRECTORY="${CMAKE_CURRENT_SOURCE_DIR}")

if(SOURCEMETA_CORE_ALLOCATOR STREQUAL "mimalloc")
target_link_libraries(sourcemeta_core_benchmark PRIVATE Mimalloc::Mimalloc)
endif()

if(SOURCEMETA_CORE_REGEX)
target_link_libraries(sourcemeta_core_benchmark
PRIVATE sourcemeta::core::regex)
Expand Down
19 changes: 19 additions & 0 deletions cmake/FindMimalloc.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
if(NOT Mimalloc_FOUND)
set(MIMALLOC_DIR "${PROJECT_SOURCE_DIR}/vendor/mimalloc")

set(MI_BUILD_SHARED OFF CACHE BOOL "" FORCE)
set(MI_BUILD_OBJECT OFF CACHE BOOL "" FORCE)
set(MI_BUILD_TESTS OFF CACHE BOOL "" FORCE)
set(MI_OVERRIDE ON CACHE BOOL "" FORCE)

add_subdirectory(
"${MIMALLOC_DIR}"
"${CMAKE_CURRENT_BINARY_DIR}/mimalloc" EXCLUDE_FROM_ALL)

if(TARGET mimalloc-static)

@jviotti jviotti Aug 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What would happen on shared builds?

set_target_properties(mimalloc-static
PROPERTIES COMPILE_WARNING_AS_ERROR OFF)
add_library(Mimalloc::Mimalloc ALIAS mimalloc-static)

@cubic-dev-ai cubic-dev-ai Bot Aug 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: When a static mimalloc with MI_OVERRIDE is linked as an ordinary archive (not whole-archive), the linker may only pull in the objects that resolve currently-undefined symbols, so the malloc/free override can silently fail on some platforms or link orders. Consider propagating whole-archive linking (e.g. target_link_options with $<LINK_LIBRARY:WHOLE_ARCHIVE,mimalloc-static>, or linking the benchmark executable with --whole-archive) so the override that the benchmark gains depend on is guaranteed and reproducible.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At cmake/FindMimalloc.cmake, line 16:

<comment>When a static mimalloc with MI_OVERRIDE is linked as an ordinary archive (not whole-archive), the linker may only pull in the objects that resolve currently-undefined symbols, so the malloc/free override can silently fail on some platforms or link orders. Consider propagating whole-archive linking (e.g. target_link_options with $<LINK_LIBRARY:WHOLE_ARCHIVE,mimalloc-static>, or linking the benchmark executable with --whole-archive) so the override that the benchmark gains depend on is guaranteed and reproducible.</comment>

<file context>
@@ -0,0 +1,19 @@
+  if(TARGET mimalloc-static)
+    set_target_properties(mimalloc-static
+      PROPERTIES COMPILE_WARNING_AS_ERROR OFF)
+    add_library(Mimalloc::Mimalloc ALIAS mimalloc-static)
+    set(Mimalloc_FOUND ON)
+  endif()
</file context>

set(Mimalloc_FOUND ON)
endif()
endif()
10 changes: 10 additions & 0 deletions vendor/mimalloc.mask
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading