-
Notifications
You must be signed in to change notification settings - Fork 4
fix(silo): reduce peak memory load by changing default allocator and tuning mimalloc options #1045
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
base: main
Are you sure you want to change the base?
Conversation
|
This is a preview of the changelog of the next release. If this branch is not up-to-date with the current main branch, the changelog may not be accurate. Rebase your branch on the main branch to get the most accurate changelog. Note that this might contain changes that are on main, but not yet released. Changelog: 0.9.2 (2025-11-10)Bug Fixes
|
1384d22 to
edd7153
Compare
…tuning mimalloc options
edd7153 to
fe1db01
Compare
|
|
||
| if (soft_memory_limit_in_kb.has_value() && rss.value() > soft_memory_limit_in_kb.value()) { | ||
| SPDLOG_INFO("Manually invoking malloc_trim() to give back memory to OS."); | ||
| malloc_trim(0); |
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.
Why are you removing this in the non-mimalloc case (making benchmarking numbers incomparable)?
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.
But this is also invoked during benchmarking or am I missing something?
| void operator>>(std::ostream& output) { | ||
| for (size_t i = 0; i < BATCH_SIZE; ++i) { | ||
| output << streams[i].rdbuf(); | ||
| writeChunked(output, std::move(streams[i]).str()); |
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.
I guess this change has nothing to do with mimalloc, but you were trying to find out where the leaking comes from?
|
|
||
| "hwloc/*:shared": False, | ||
|
|
||
| "mimalloc/*:override": True, |
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.
What does this do? (Can't find it on https://conan.io/center/recipes/mimalloc)
| #ifdef SILO_USE_MIMALLOC | ||
| // If this option is not set, memory remains very high even when no requests are sent | ||
| // Also reduces peak memory usage under concurrency | ||
| mi_option_set(mi_option_purge_delay, 0); |
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.
Worrisome with regards to performance: I guess fixes the "leaking" but at the cost of speed: "Setting N to 0 purges immediately when a page becomes unused which can improve memory usage but also decreases performance." (https://github.com/microsoft/mimalloc/blob/main/doc/mimalloc-doc.h)
reduces impact of #1034
Summary
We were already using
mimallocindirectly with Arrow (e.g. through ArrowRecordBatchcreating). Now we are usingmimallocfor all allocations in SILO.I tested this by starting an instance with loculus data and stress testing the sequence download:
Before:
After:
I let the stress tests running for >10 minutes and the memory did not increase above the peak anymore