Try to use mimalloc allocator #37159
Conversation
l1tsolaiki
force-pushed
the
l1tsolaiki-mimalloc
branch
from
0406326 to
53b8db7
Compare
robot-clickhouse
added
the
submodule changed
nikitamikhaylov
added
the
can be tested
azat
left a comment
azat
left a comment
There was a problem hiding this comment.
By the way, what is the purpose of using mimalloc?
src/Common/memory.h
Outdated
There was a problem hiding this comment.
Debugging stuff ok, but I guess it will be enough to handle mimalloc first and jemalloc as a fallback
There was a problem hiding this comment.
I don't see mimalloc-static anywhere in this cmake rules.
There was a problem hiding this comment.
I am following guide from mimalloc/README.md. Looks like it is defined somewhere inside mimalloc itself.
Furthermore, it is done in the same way in previous attempt to use mimalloc. (#5775)
There was a problem hiding this comment.
You don't neither MIMALLOC_LIBRARY, so you don't need this variable. And AFAICS right now you added only header only library, so it does not matter static or shared.
Also note (for the future), that there is module part too, i.e. not headers only. Seems that it is required for overrides - https://github.com/microsoft/mimalloc#static-override
src/CMakeLists.txt
Outdated
There was a problem hiding this comment.
By the way, you have not disabled jemalloc, hence mimalloc is not used in the build
There is an issue #34157 for testing different allocators, mimalloc being one of them. So the purpose of this PR is to test its performance. |
robot-clickhouse
added
the
pr-performance
l1tsolaiki
force-pushed
the
l1tsolaiki-mimalloc
branch
from
db4d03f to
95ea5a9
Compare
…quota to previously failing test
| # Note: turn of this warning: | ||
| # error: identifier '_mi_option_last' is reserved because it starts with '_' at global scope | ||
| ) | ||
| target_include_directories(_mimalloc SYSTEM BEFORE PUBLIC "${ClickHouse_SOURCE_DIR}/contrib/mimalloc/include") |
There was a problem hiding this comment.
You forgot MI_MALLOC_OVERRIDE, i.e. target_compile_definitions(_mimalloc PRIVATE -DMI_MALLOC_OVERRIDE), this should fix incorrect allocator usage (and possible SIGSEGV due to this).
There was a problem hiding this comment.
But this will not be enough, microsoft/mimalloc#583 is required, I've submitted separate draft PR on top of yours that includes all review changes (I cannot push to your fork) - #37304
| target_compile_options(_mimalloc | ||
| PRIVATE | ||
| -Wno-reserved-identifier | ||
| # Note: turn of this warning: | ||
| # error: identifier '_mi_option_last' is reserved because it starts with '_' at global scope |
There was a problem hiding this comment.
Not important, but this looks better:
| target_compile_options(_mimalloc | |
| PRIVATE | |
| -Wno-reserved-identifier | |
| # Note: turn of this warning: | |
| # error: identifier '_mi_option_last' is reserved because it starts with '_' at global scope | |
| target_compile_options(_mimalloc PRIVATE | |
| # Note: turn of this warning: | |
| # error: identifier '_mi_option_last' is reserved because it starts with '_' at global scope | |
| -Wno-reserved-identifier |
l1tsolaiki
force-pushed
the
l1tsolaiki-mimalloc
branch
from
6f786bb to
7ecd82f
Compare
|
@azat It is using 2.5 times more resident memory according to performance tests. |
5 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Try to use
mimallocallocator