-
Notifications
You must be signed in to change notification settings - Fork 944
new(cmake,userspace,ci): add mimalloc support #3616
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
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FedeDP The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
9fe5a59
to
e912c8c
Compare
userspace/falco/CMakeLists.txt
Outdated
|
||
target_link_libraries(falco_application ${FALCO_LIBRARIES}) | ||
|
||
target_link_libraries(falco_application PUBLIC ${FALCO_LIBRARIES}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the link libraries PUBLIC so that they are also part of the falco_application
interface.
No need to specify them later while creating the Falco executable.
userspace/falco/CMakeLists.txt
Outdated
target_include_directories(falco_application PUBLIC ${FALCO_INCLUDE_DIRECTORIES}) | ||
|
||
add_executable(falco falco.cpp) | ||
add_dependencies(falco falco_application ${FALCO_DEPENDENCIES}) | ||
target_link_libraries(falco falco_application ${FALCO_LIBRARIES}) | ||
add_dependencies(falco falco_application) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transitive dep: falco_application
already depends upon FALCO_DEPENDENCIES
.
# reproducible" while building mimalloc, we force-set both variables. | ||
string(TIMESTAMP DATE "%Y%m%d") | ||
string(TIMESTAMP TIME "%H:%M") | ||
set(MIMALLOC_EXTRA_CPPDEFS __DATE__="${DATE}",__TIME__="${TIME}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to force-set __DATE__
and __TIME__
macro to avoid a build failure with
2025-07-22T09:24:49.5049071Z /home/runner/work/falco/falco/build/mimalloc-prefix/src/malloc/src/options.c:226:9: error: expansion of date or time macro is not reproducible [-Werror,-Wdate-time]
2025-07-22T09:24:49.5076642Z 226 | , __DATE__, __TIME__);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idea here is that all the allocator library we might eventually support (for now jemalloc and mimalloc) will create a malloc
target and set MALLOC_LIB
.
Then, main CMakeLists will add a dep on the malloc
target for falco_application
and will link MALLOC_LIB
too.
At the same time, it also gives a desirable side effect that if multiple allocators gets enabled in the same build, we'll have multiple malloc
targets defined leading to an automatic cmake configuration error.
Signed-off-by: Federico Di Pierro <[email protected]>
…able jemalloc or mimalloc. Enable mimalloc in all CIs but release CI (keep it with jemalloc for now). Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
Signed-off-by: Federico Di Pierro <[email protected]>
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area build
/area CI
What this PR does / why we need it:
Added mimalloc support.
It is a mem allocator by Microsoft. Also, to be able to test it on our infra cluster, i enabled mimalloc instead of jemalloc on CI and master builds.
Release builds are kept with jemalloc for now.
NOTE: jemalloc project has been archived too: https://jasone.github.io/2025/06/12/jemalloc-postmortem/
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: