feat: enable jemalloc for Mooncake Store master by default#1436
feat: enable jemalloc for Mooncake Store master by default#1436acelyc111 wants to merge 2 commits intokvcache-ai:mainfrom
Conversation
acelyc111
requested review from
ShangmingCai,
XucSh,
YiXR,
stmatengss and
ykwd
as code owners
Summary of ChangesHello @acelyc111, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request aims to enhance the stability and performance of the Mooncake Store master by enabling the jemalloc memory allocator by default. This change is crucial for high-load scenarios where frequent object insertions and removals from unordered maps can lead to out-of-memory errors. By making jemalloc the default, the system benefits from more efficient memory allocation, reduced fragmentation, and improved memory return to the operating system, thereby proactively preventing common memory-related issues for users. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enables jemalloc by default for the Mooncake Store master to improve memory management, especially in high-load scenarios. The changes correctly update the CMake build options and add documentation for the new default setting.
My main feedback is to also update the dependency lists in the build documentation. Since jemalloc is now a default dependency, users following the manual build instructions will need to install it to avoid build failures. I've added comments to the documentation files with this suggestion.
There was a problem hiding this comment.
Pull request overview
This pull request enables jemalloc as the default memory allocator for the Mooncake Store master component to address out-of-memory (OOM) issues in high-load scenarios where frequent insertions and removals occur in the master's unordered_map data structures.
Changes:
- Changed the default value of
STORE_USE_JEMALLOCCMake option from OFF to ON - Added comprehensive documentation comments explaining jemalloc's benefits (memory return to OS, multi-threaded performance, fragmentation reduction)
- Updated build documentation to reflect the new default setting
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| CMakeLists.txt | Changed STORE_USE_JEMALLOC default from OFF to ON and added status message |
| mooncake-store/src/CMakeLists.txt | Added detailed comments explaining jemalloc benefits for mooncake_master |
| docs/source/getting_started/build.md | Updated documentation to indicate default is ON |
| docs/source/zh_archive/build.md | Updated Chinese documentation to indicate default is ON |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
| pybind11-dev \ | ||
| libcurl4-openssl-dev \ | ||
| libhiredis-dev \ | ||
| libjemalloc-dev \ |
There was a problem hiding this comment.
For building the whole project, I think libjemalloc-dev should be an optional dependency.
There was a problem hiding this comment.
We can install the dependencies when prepare the build environment, whether to enable the option depends on the user. Pre-install the dependency could mute the complains like 'why it needs another dependency when I enable the compile option'?
|
LGTM |
|
@ykwd What's your opinion? |
|
@ykwd PTAL, thanks! |
|
Only one concern: Jemalloc should be a general package available across Linux distributions. It's currently unavailable through package managers like Suse's. |
3 participants
Description
Jemalloc is very important in high load senarios, it would encounter OOM frequently when insert in and remove keys from master unorderedmap.
We can provide Jemalloc version by default to prevent users encounter this issue.
before:
after:
Type of Change
How Has This Been Tested?
Checklist
./scripts/code_format.shbefore submitting.