mimalloc integration - #2708
Conversation
Signed-off-by: Vansh <officialbusiness9818@gmail.com>
98b80e7 to
66cfe1d
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… benchmark CMakeLists Signed-off-by: Vansh <officialbusiness9818@gmail.com>
There was a problem hiding this comment.
2 issues found across 58 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmake/FindMimalloc.cmake">
<violation number="1" location="cmake/FindMimalloc.cmake:16">
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.</violation>
</file>
<file name="CMakeLists.txt">
<violation number="1" location="CMakeLists.txt:77">
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.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if(TARGET mimalloc-static) | ||
| set_target_properties(mimalloc-static | ||
| PROPERTIES COMPILE_WARNING_AS_ERROR OFF) | ||
| add_library(Mimalloc::Mimalloc ALIAS mimalloc-static) |
There was a problem hiding this comment.
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>
|
|
||
| # TODO: Turn this into a re-usable utility CMake function | ||
| if(SOURCEMETA_CORE_ALLOCATOR STREQUAL "mimalloc") | ||
| find_package(Mimalloc REQUIRED) |
There was a problem hiding this comment.
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>
| .cache | ||
| out/ | ||
| CMakeSettings.json | ||
|
|
| list(APPEND CMAKE_MODULE_PATH "${PROJECT_SOURCE_DIR}/cmake") | ||
|
|
||
| # Options | ||
| #Options |
There was a problem hiding this comment.
| #Options | |
| # Options |
| endif() | ||
|
|
||
| # Enable the sanitizers before defining any target | ||
| #Enable the sanitizers before defining any target |
There was a problem hiding this comment.
Looks like in general most comments lost the initial space for some reason?
There was a problem hiding this comment.
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.
| include(Sourcemeta) | ||
|
|
||
| # Don't force downstream consumers on this | ||
| sourcemeta_option_enum( |
There was a problem hiding this comment.
If minalloc is indeed consistently faster, then no need to have it as an option. Let's just compile it unconditionally?
| "${MIMALLOC_DIR}" | ||
| "${CMAKE_CURRENT_BINARY_DIR}/mimalloc" EXCLUDE_FROM_ALL) | ||
|
|
||
| if(TARGET mimalloc-static) |
There was a problem hiding this comment.
What would happen on shared builds?
|
Very interesting! Given the speed bumps, left some comments about integrating it unconditionally. Probably no need to support multiple allocators. If this one is good, then let's just use it everywhere? |
|
Right now If we link it unconditionally across the whole project, every single DLL will embed its own independent static copy of mimalloc, which causes crashes. it would work perfectly on unix but to make mimalloc globally override the system allocator on Windows, we will have to use special linking flags, force dynamic overrides, and carefully manage DLLs, so basically there is an extra risk factor i suggest keeping mimalloc as optional as of right now, while if you want me to, i can work on making it unconditional and submit it as a separate PR in the future. |
|
@Vansh-kap-98 What about we make it unconditionally ONLY for Linux to start with, as a first step? So on Linux, it's unconditional. The rest uses the default allocator. The reason I'm pushing for not having the additional option is that it's a whole other alternate build configuration to maintain, with more additions to the CI matrix, etc. I much rather have a "blessed" configuration all projects operate on. I tried it on my Mac and its not so easy there (there are caveats just like on Windows), so we can start Linux only, and try future PRs to make it work in the rest? |
|
@jviotti understood, ill remove the "optional" part and integrate it unconditionally for linux for this specific PR |
|
Awesome, thanks! Let me know once the changes are made for me to take a look. Excited to land this! |
Signed-off-by: Vansh <officialbusiness9818@gmail.com>
|
|
There was a problem hiding this comment.
3 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="patches/mimalloc/0001-Skip-vendored-install-rules-when-embedded.patch">
<violation number="1" location="patches/mimalloc/0001-Skip-vendored-install-rules-when-embedded.patch:11">
P3: The patch guards mimalloc's `install(TARGETS ...)` and `install(EXPORT ...)` rules with `MI_SKIP_INSTALL`, but the vendored CMakeLists still contains unguarded `install(FILES include/mimalloc*.h ...)` and `install(FILES cmake/mimalloc-config*.cmake ...)` rules (vendor/mimalloc/CMakeLists.txt lines 768-774). Since FindMimalloc.cmake sets `MI_SKIP_INSTALL ON` during `add_subdirectory` and then re-registers its own installs of those same headers/config files to the same destinations, `cmake --install` will execute the vendored rules too, installing duplicates of identical files. The guard's stated intent ('skip vendored install rules when embedded') is only partially achieved. Consider extending the guard (or the install trigger) so the vendored `install(FILES ...)` rules are also skipped, leaving a single source of install rules in FindMimalloc.cmake.</violation>
</file>
<file name="cmake/common/targets/library.cmake">
<violation number="1" location="cmake/common/targets/library.cmake:69">
P2: This `sourcemeta_library` helper lives in the shared `cmake/common/` module set used across sourcemeta projects, but it now unconditionally links the raw `mimalloc` / `mimalloc-static` targets on Linux for every library. Those targets only exist because this repo's top-level `CMakeLists.txt` happens to call `find_package(Mimalloc REQUIRED)` and `add_subdirectory(vendor/mimalloc)` first. Any consumer that pulls in these common modules on Linux without first configuring the vendored mimalloc will get a hard configure-time failure (linking a non-existent target), and the allocator now becomes a hard requirement of every library build even where the override is not needed. Consider making the link conditional on the target actually existing (e.g. `if(TARGET mimalloc-static OR TARGET mimalloc)`) or gating it behind a feature flag that this PR can enable, so the shared module stays usable independently.</violation>
</file>
<file name=".gitattributes">
<violation number="1" location=".gitattributes:4">
P3: The diff shows `\ No newline at end of file` after the new `test/test/*.txt text eol=lf` line, and `.gitignore` likewise dropped its trailing blank line and now also ends without a newline. POSIX tools and git generally expect a trailing newline on text files. Add the trailing newline (and keep the removed trailing blank line if it was intentional) to keep the files clean.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| add_library(${ALIAS_NAME} ALIAS ${TARGET_NAME}) | ||
|
|
||
| if(SOURCEMETA_OS_LINUX) |
There was a problem hiding this comment.
P2: This sourcemeta_library helper lives in the shared cmake/common/ module set used across sourcemeta projects, but it now unconditionally links the raw mimalloc / mimalloc-static targets on Linux for every library. Those targets only exist because this repo's top-level CMakeLists.txt happens to call find_package(Mimalloc REQUIRED) and add_subdirectory(vendor/mimalloc) first. Any consumer that pulls in these common modules on Linux without first configuring the vendored mimalloc will get a hard configure-time failure (linking a non-existent target), and the allocator now becomes a hard requirement of every library build even where the override is not needed. Consider making the link conditional on the target actually existing (e.g. if(TARGET mimalloc-static OR TARGET mimalloc)) or gating it behind a feature flag that this PR can enable, so the shared module stays usable independently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmake/common/targets/library.cmake, line 69:
<comment>This `sourcemeta_library` helper lives in the shared `cmake/common/` module set used across sourcemeta projects, but it now unconditionally links the raw `mimalloc` / `mimalloc-static` targets on Linux for every library. Those targets only exist because this repo's top-level `CMakeLists.txt` happens to call `find_package(Mimalloc REQUIRED)` and `add_subdirectory(vendor/mimalloc)` first. Any consumer that pulls in these common modules on Linux without first configuring the vendored mimalloc will get a hard configure-time failure (linking a non-existent target), and the allocator now becomes a hard requirement of every library build even where the override is not needed. Consider making the link conditional on the target actually existing (e.g. `if(TARGET mimalloc-static OR TARGET mimalloc)`) or gating it behind a feature flag that this PR can enable, so the shared module stays usable independently.</comment>
<file context>
@@ -66,6 +66,27 @@ function(sourcemeta_library)
add_library(${ALIAS_NAME} ALIAS ${TARGET_NAME})
+ if(SOURCEMETA_OS_LINUX)
+ if(BUILD_SHARED_LIBS)
+ set(SOURCEMETA_MIMALLOC_LINK_LIBRARY mimalloc)
</file context>
| ) | ||
| - install(TARGETS mimalloc EXPORT mimalloc ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR} LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}) | ||
| - install(EXPORT mimalloc DESTINATION ${mi_install_cmakedir}) | ||
| + if(NOT MI_SKIP_INSTALL) |
There was a problem hiding this comment.
P3: The patch guards mimalloc's install(TARGETS ...) and install(EXPORT ...) rules with MI_SKIP_INSTALL, but the vendored CMakeLists still contains unguarded install(FILES include/mimalloc*.h ...) and install(FILES cmake/mimalloc-config*.cmake ...) rules (vendor/mimalloc/CMakeLists.txt lines 768-774). Since FindMimalloc.cmake sets MI_SKIP_INSTALL ON during add_subdirectory and then re-registers its own installs of those same headers/config files to the same destinations, cmake --install will execute the vendored rules too, installing duplicates of identical files. The guard's stated intent ('skip vendored install rules when embedded') is only partially achieved. Consider extending the guard (or the install trigger) so the vendored install(FILES ...) rules are also skipped, leaving a single source of install rules in FindMimalloc.cmake.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At patches/mimalloc/0001-Skip-vendored-install-rules-when-embedded.patch, line 11:
<comment>The patch guards mimalloc's `install(TARGETS ...)` and `install(EXPORT ...)` rules with `MI_SKIP_INSTALL`, but the vendored CMakeLists still contains unguarded `install(FILES include/mimalloc*.h ...)` and `install(FILES cmake/mimalloc-config*.cmake ...)` rules (vendor/mimalloc/CMakeLists.txt lines 768-774). Since FindMimalloc.cmake sets `MI_SKIP_INSTALL ON` during `add_subdirectory` and then re-registers its own installs of those same headers/config files to the same destinations, `cmake --install` will execute the vendored rules too, installing duplicates of identical files. The guard's stated intent ('skip vendored install rules when embedded') is only partially achieved. Consider extending the guard (or the install trigger) so the vendored `install(FILES ...)` rules are also skipped, leaving a single source of install rules in FindMimalloc.cmake.</comment>
<file context>
@@ -0,0 +1,30 @@
+ )
+- install(TARGETS mimalloc EXPORT mimalloc ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR} LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR})
+- install(EXPORT mimalloc DESTINATION ${mi_install_cmakedir})
++ if(NOT MI_SKIP_INSTALL)
++ install(TARGETS mimalloc EXPORT mimalloc ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR} LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR})
++ install(EXPORT mimalloc DESTINATION ${mi_install_cmakedir})
</file context>
| /test/yaml/stubs/** -text | ||
| vendor/*.mask text eol=lf | ||
| *.sh text eol=lf | ||
| test/test/*.txt text eol=lf No newline at end of file |
There was a problem hiding this comment.
P3: The diff shows \ No newline at end of file after the new test/test/*.txt text eol=lf line, and .gitignore likewise dropped its trailing blank line and now also ends without a newline. POSIX tools and git generally expect a trailing newline on text files. Add the trailing newline (and keep the removed trailing blank line if it was intentional) to keep the files clean.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .gitattributes, line 4:
<comment>The diff shows `\ No newline at end of file` after the new `test/test/*.txt text eol=lf` line, and `.gitignore` likewise dropped its trailing blank line and now also ends without a newline. POSIX tools and git generally expect a trailing newline on text files. Add the trailing newline (and keep the removed trailing blank line if it was intentional) to keep the files clean.</comment>
<file context>
@@ -1,2 +1,4 @@
/vendor/** linguist-generated=true
vendor/*.mask text eol=lf
+*.sh text eol=lf
+test/test/*.txt text eol=lf
\ No newline at end of file
</file context>
| test/test/*.txt text eol=lf | |
| test/test/*.txt text eol=lf | |
2 participants
Overview
This PR integrates
mimallocas an optional allocator path to significantly improve memory allocation performance during core benchmarking. The default allocator behavior remains unchanged for standard execution, ensuring stability while allowing us to specifically test and leveragemimallocwhere intended.1. Dependency and Vendoring Changes
mimallocas a pinned vendored dependency inDEPENDENCIES.mimalloc.mask) to strip non-essential upstream content duringvendorpull. This keeps the vendored tree minimal for the repository's build use (cmake,include,src, etc.) and avoids carrying extra upstream documentation, tests, or tooling.2. Build-System Integration
FindMimalloc.cmake.MIMALLOC_DIR.mimallocwith the intended options (static build path, tests/object output disabled, override enabled).add_subdirectorysomimallocis correctly built by its own upstream CMake logic.Mimalloc::Mimallocfor clean linking by project targets.3. Results and findings
Aggregate of 10 runs for each, system vs mimalloc, major comparisons are as follows (><5%)
1. Regex Operations
Regex_Lower_S_Or_Upper_S_AsteriskRegex_Caret_Lower_S_Or_Upper_S_Asterisk_DollarRegex_Period_AsteriskRegex_Group_Period_Asterisk_GroupRegex_Period_PlusRegex_PeriodRegex_Caret_Period_Plus_DollarRegex_Caret_Group_Period_Plus_Group_DollarRegex_Caret_Period_Asterisk_DollarRegex_Caret_Group_Period_Asterisk_Group_DollarRegex_Caret_X_HyphenRegex_Period_Md_DollarRegex_Caret_Slash_Period_AsteriskRegex_Caret_Period_Range_DollarRegex_Nested_Backtrack2. JSON & JSON-LD Operations
JSON_Array_Of_Objects_UniqueJSON_Parse_1JSON_Parse_RealJSON_Parse_DecimalJSON_Parse_Schema_ISO_LanguageJSON_Parse_IntegerJSON_Parse_String_NonSSO_PlainJSON_Parse_String_SSO_PlainJSON_Parse_String_Escape_HeavyJSON_Parse_Object_Short_KeysJSON_Parse_Object_Scalar_PropertiesJSON_Parse_Object_Array_PropertiesJSON_Parse_Object_Object_PropertiesJSON_Parse_Nested_ContainersJSON_From_String_CopyJSON_From_String_TemporaryJSON_Number_To_DoubleJSON_String_Equal_Small_By_Runtime_Perfect_Hash/10JSON_String_Fast_Hash/10JSON_String_Fast_Hash/100JSON_String_Key_Hash/10JSONL_Parse_LargeJSONLD_Catalog_Annotation_List_PopulateJSONLD_Catalog_Materialize3. Pointer & JSONPath Operations
Pointer_Object_TraversePointer_Object_Try_TraversePointer_Push_Back_Pointer_To_Weak_PointerPointer_Walker_Schema_ISO_LanguagePointer_Maybe_Tracked_Deeply_Nested/0Pointer_Maybe_Tracked_Deeply_Nested/1Pointer_Position_Tracker_Get_Deeply_NestedJSONPath_Descendant_Filter_Nested4. URI Template Router Operations
URITemplateRouter_CreateURITemplateRouter_MatchURITemplateRouter_Match_BasePathURITemplateRouterView_RestoreURITemplateRouterView_MatchURITemplateRouterView_Match_BasePathURITemplateRouterView_Arguments