Skip to content

cmake: fix support for je/mi malloc and the mallocallocator#10407

Merged
cmcfarlen merged 4 commits intoapache:masterfrom
cmcfarlen:cmake-malloc-allocator
Sep 25, 2023
Merged

cmake: fix support for je/mi malloc and the mallocallocator#10407
cmcfarlen merged 4 commits intoapache:masterfrom
cmcfarlen:cmake-malloc-allocator

Conversation

@cmcfarlen
Copy link
Copy Markdown
Contributor

fixes #10377

@cmcfarlen cmcfarlen added the CMake work related to CMakes scripts or issues label Sep 13, 2023
@cmcfarlen cmcfarlen added this to the 10.0.0 milestone Sep 13, 2023
@cmcfarlen cmcfarlen self-assigned this Sep 13, 2023
Copy link
Copy Markdown
Contributor

@JosiahWI JosiahWI left a comment

Choose a reason for hiding this comment

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

Looks good!

@cmcfarlen cmcfarlen force-pushed the cmake-malloc-allocator branch from f23e44a to 70bfe0a Compare September 18, 2023 15:58
@bryancall bryancall self-requested a review September 18, 2023 22:22
@cmcfarlen cmcfarlen force-pushed the cmake-malloc-allocator branch from 70bfe0a to bd792df Compare September 20, 2023 15:23
Comment thread CMakeLists.txt
link_libraries(jemalloc)
link_libraries(jemalloc::jemalloc)
elseif(TS_HAS_MIMALLOC)
link_libraries(mimalloc)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
link_libraries(mimalloc)
link_libraries(mimalloc::mimalloc)

here too

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There isn't currently a mimalloc::mimalloc target

@bryancall
Copy link
Copy Markdown
Contributor

Testing malloc:

$ cmake -B build -G Ninja -DCMAKE_INSTALL_PREFIX=/opt/ats -DCMAKE_BUILD_TYPE=Debug -DENABLE_AUTEST=ON -DBUILD_EXPERIMENTAL_PLUGINS=ON -DENABLE_MALLOC_ALLOCATOR=ON
$ cd build; ninja
$ grep -ir TS_USE_MALLOC_ALLOCATOR
include/tscore/ink_config.h:#define TS_USE_MALLOC_ALLOCATOR 1

@bryancall
Copy link
Copy Markdown
Contributor

By default it looks like we enable jemalloc. I don't believe this is what we do for autotools:

CMakeCache.txt:ENABLE_JEMALLOC:STRING=AUTO
CMakeCache.txt:jemalloc_INCLUDE_DIR:PATH=/usr/include/jemalloc
CMakeCache.txt:jemalloc_LIBRARY:FILEPATH=/usr/lib64/libjemalloc.so

#if __has_include(<jemalloc/jemalloc.h>)
#include <jemalloc/jemalloc.h>
#else
#include <jemalloc.h>
Copy link
Copy Markdown
Contributor

@bryancall bryancall Sep 25, 2023

Choose a reason for hiding this comment

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

We only need to have #include <jemalloc.h> in the code for compiling with cmake. The include directory detection add in the subdir jemalloc.

However, with autotools we need to have the #include <jemalloc/jemalloc.h>

@cmcfarlen cmcfarlen merged commit d7f27c4 into apache:master Sep 25, 2023
@cmcfarlen cmcfarlen deleted the cmake-malloc-allocator branch September 25, 2023 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CMake work related to CMakes scripts or issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cmake: JE/MI malloc support

3 participants