Integrate with mimalloc on Linux - #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.
1 issue 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>
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>
| .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.
All reported issues were addressed across 9 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
still making some changes and reviews, just wanted to see the tests and cubic responses as im using docker and wsl to run the tests, ill tag you once i complete the task and verify all files |
|
Awesome! And Yeah, feel free to iterate with CI here as much as you need to :) |
Signed-off-by: Vansh <officialbusiness9818@gmail.com>
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…omplete vendored install-skip coverage Signed-off-by: Vansh <officialbusiness9818@gmail.com>
There was a problem hiding this comment.
1 issue found across 5 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:91">
P2: The final hunk of this patch declares `@@ -819,8 +831,10 @@`, but its content is only 5 old lines and 7 new lines, so strict `git apply` rejects the entire patch as corrupt (`error: corrupt patch` at EOF). The count should be `-819,5 +831,7`. If the vendorpull flow uses `git apply`, pulling mimalloc breaks; if it uses lenient GNU `patch`, it is silently tolerated. Fix the hunk header counts to make the patch a well-formed unified diff regardless of the apply tool.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| -install(FILES "${CMAKE_CURRENT_BINARY_DIR}/mimalloc.pc" | ||
| - DESTINATION "${CMAKE_INSTALL_LIBDIR}/pkgconfig/") | ||
| +if(NOT MI_SKIP_INSTALL) | ||
| + install(FILES "${CMAKE_CURRENT_BINARY_DIR}/mimalloc.pc" |
There was a problem hiding this comment.
P2: The final hunk of this patch declares @@ -819,8 +831,10 @@, but its content is only 5 old lines and 7 new lines, so strict git apply rejects the entire patch as corrupt (error: corrupt patch at EOF). The count should be -819,5 +831,7. If the vendorpull flow uses git apply, pulling mimalloc breaks; if it uses lenient GNU patch, it is silently tolerated. Fix the hunk header counts to make the patch a well-formed unified diff regardless of the apply tool.
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 91:
<comment>The final hunk of this patch declares `@@ -819,8 +831,10 @@`, but its content is only 5 old lines and 7 new lines, so strict `git apply` rejects the entire patch as corrupt (`error: corrupt patch` at EOF). The count should be `-819,5 +831,7`. If the vendorpull flow uses `git apply`, pulling mimalloc breaks; if it uses lenient GNU `patch`, it is silently tolerated. Fix the hunk header counts to make the patch a well-formed unified diff regardless of the apply tool.</comment>
<file context>
@@ -46,3 +66,28 @@ index 3189dec42..565852ecd 100644
+-install(FILES "${CMAKE_CURRENT_BINARY_DIR}/mimalloc.pc"
+- DESTINATION "${CMAKE_INSTALL_LIBDIR}/pkgconfig/")
++if(NOT MI_SKIP_INSTALL)
++ install(FILES "${CMAKE_CURRENT_BINARY_DIR}/mimalloc.pc"
++ DESTINATION "${CMAKE_INSTALL_LIBDIR}/pkgconfig/")
++endif()
</file context>
Signed-off-by: Vansh <officialbusiness9818@gmail.com>
|
Hi @jviotti ,this should be ready for review now. CI is green across the full matrix including the sanitizer job. Let me know if you'd like anything adjusted. edit: i forgot to mention |
There was a problem hiding this comment.
Are these changes really needed? Seem unrelated?
There was a problem hiding this comment.
yes they are quite unrelated but i thought i should atleast point them out instead of just ignoring them, better safe than sorry
| Brewfile.lock.json | ||
| .DS_Store | ||
| .cache | ||
| out/ |
There was a problem hiding this comment.
| out/ | |
| /out/ |
To prevent any directory called out in the future anywhere else nested in this project being silently ignored
| if(SOURCEMETA_OS_LINUX | ||
| AND NOT SOURCEMETA_CORE_ADDRESS_SANITIZER | ||
| AND NOT SOURCEMETA_CORE_UNDEFINED_SANITIZER) | ||
| set(SOURCEMETA_CORE_MIMALLOC_ENABLED ON) |
There was a problem hiding this comment.
We could move this to the Find script
|
Looks good. Mostly just very minor comments. Let me just push a commit here myself so we can merge this in a bit |
There was a problem hiding this comment.
I think we can get rid of this by setting up the dependency ourselves in the find script like for the others. But something to do in a subsequent PR to make sure performance is not sacrificed
|
Thanks a lot! Some of the things I'll to explore soon:
|
|
@jviotti thanks a lot for allowing me to work on it, and im looking forward to working on this even more, i appreciate all the time and effort you put in reviewing my commits and providing insights. Ive noted everything and will make sure to work on them in the very near future and keep you updated. |
|
@Vansh-kap-98 Any time! Excellent work and research behind this. This kind of stuff is super appreciated and massively impactful. If you like the performance research aspect of it, I would love your eyes on how do you think we could make the https://github.com/sourcemeta/blaze evaluator faster? i.e. this part: https://github.com/sourcemeta/blaze/tree/main/src/evaluator We have plenty of benchmark cases for the evaluator (though other stuff too). |
|
@jviotti okie, ill make sure to take a look and keep you updated |
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