-
Notifications
You must be signed in to change notification settings - Fork 204
feat: add jemalloc as optional custom allocator #1679
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
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.
LGTM. Thanks @mbutrovich
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1679 +/- ##
============================================
+ Coverage 56.12% 58.81% +2.68%
- Complexity 976 1082 +106
============================================
Files 119 125 +6
Lines 11743 12597 +854
Branches 2251 2362 +111
============================================
+ Hits 6591 7409 +818
- Misses 4012 4016 +4
- Partials 1140 1172 +32 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The Spark SQL test does not seem related to the change. |
Restarted the test |
# Conflicts: # native/Cargo.lock # native/core/Cargo.toml
Which issue does this PR close?
Closes #1648.
Rationale for this change
Comet currently supports mimalloc to override the system memory allocator (
make release COMET_FEATURES=mimalloc
orcargo build --features=mimalloc
). We want to explore jemalloc (tikv-jemallocator crate) as an alternative to mimalloc for our custom allocator. I will bring some discussion from #1648:#1648 (comment)
#1648 (comment)
#1648 (comment)
Other relevant reading:
What changes are included in this PR?
Add jemalloc as a feature accessible through
make release COMET_FEATURES=jemalloc
. I had tcmalloc (Google) and snmalloc (Microsoft Research) as well, but the former failed to link at compilation time and the latter didn't like being loaded later as a dynamic library with Comet.How are these changes tested?