tags 914814 - help + patch
thanks
Hi there,
jemalloc (wannabe) maintainer here :) Thanks Adam for filing this!
On Sat, Dec 01, 2018 at 07:34:45AM +0100, Andreas Tille wrote:
> Spades is originally carrying a code copy. I'm afraid upstream will
> give the advise to use the code they are shipping which we want to
> avoid. Could you possibly provide some patch which I could use and
> provide to upstream once tested? Unfortunately I have no idea about
> jemalloc.
It looks like spades is using rallocm(), which was an experimental API
that was deprecated in 3.5.0 in favor of the *allocx variants, and
eventually removed in 4.0. (Debian currently carries 3.6.0, and the
intention is to move to 5.1.0.)
The whole block is a bit odd, as it's trying to grow the allocation
in-place and then fall back to malloc/memcpy/free... which is really
something that the allocator (any allocator!) can just do automatically
with (je_)realloc(). Unfortunately the class' method is already poorly
named as "realloc" and conflicts with a realloc() invocation. While
upstream renaming that method to avoid the conflict would be a trivial
fix, it would be a more invasive patch that I'd be comfortable with to
carry as a Debian patch.
Therefore attached is a patch that addresses this using the je_rallocx
API. It also deals with the deprecation of the stats.cactive statistic
(as of jemalloc 4.0), using the stats.active instead.
The patch should be 3.6.0-compatible as well and can go in anytime and
ahead of a potential jemalloc transition. It is lightly tested (i.e.
builds and runs the test suite).
More broadly, my recommendation to upstream would be to drop the ancient
(and patched?) embedded copy of jemalloc, use up-to-date standard API
interfaces (malloc/realloc) and opportunistically use the jemalloc
allocator when present without any special calls to it other than
perhaps the mallctl stats.
Regards,
Faidon
--- a/src/common/adt/kmer_vector.hpp
+++ b/src/common/adt/kmer_vector.hpp
@@ -26,18 +26,12 @@ private:
ElTy *realloc() {
#ifdef SPADES_USE_JEMALLOC
- // First, try to expand in-place
- if (storage_ && sizeof(ElTy) * capacity_ * el_sz_ > 4096 &&
- je_rallocm((void **) &storage_, NULL, sizeof(ElTy) * capacity_ * el_sz_, 0, ALLOCM_NO_MOVE) ==
- ALLOCM_SUCCESS)
- return storage_;
-
- // Failed, do usual malloc / memcpy / free cycle
- ElTy *res = (ElTy *) je_malloc(sizeof(ElTy) * capacity_ * el_sz_);
- if (storage_)
- std::memcpy(res, storage_, size_ * sizeof(ElTy) * el_sz_);
- je_free(storage_);
- storage_ = res;
+ // could simply be je_realloc(), if the realloc name wasn't redefined here
+ if (storage_) {
+ storage_ = (ElTy *) je_rallocx(storage_, sizeof(ElTy) * capacity_ * el_sz_, 0);
+ } else {
+ storage_ = (ElTy *) je_malloc(sizeof(ElTy) * capacity_ * el_sz_);
+ }
#else
// No JEMalloc, no cookies
ElTy *res = new ElTy[capacity_ * el_sz_];
--- a/src/common/utils/logger/logger_impl.cpp
+++ b/src/common/utils/logger/logger_impl.cpp
@@ -106,17 +106,13 @@ void logger::log(level desired_level, const char* file, size_t line_num, const c
size_t max_rss;
#ifdef SPADES_USE_JEMALLOC
- const size_t *cmem = 0;//, *cmem_max = 0;
+ size_t cmem = 0;
size_t clen = sizeof(cmem);
- je_mallctl("stats.cactive", &cmem, &clen, NULL, 0);
- //je_mallctl("stats.cactive_max", &cmem_max, &clen, NULL, 0);
- mem = (*cmem) / 1024;
- //max_rss = (*cmem_max) / 1024;
- max_rss = utils::get_max_rss();
-#else
- max_rss = utils::get_max_rss();
+ je_mallctl("stats.active", (void *)&cmem, &clen, NULL, 0);
+ mem = cmem / 1024;
#endif
+ max_rss = utils::get_max_rss();
for (auto it = writers_.begin(); it != writers_.end(); ++it)
(*it)->write_msg(time, mem, max_rss, desired_level, file, line_num, source, msg);
--- a/src/common/utils/memory_limit.cpp
+++ b/src/common/utils/memory_limit.cpp
@@ -83,11 +83,11 @@ size_t get_max_rss() {
size_t get_used_memory() {
#ifdef SPADES_USE_JEMALLOC
- const size_t *cmem = 0;
+ size_t cmem;
size_t clen = sizeof(cmem);
- je_mallctl("stats.cactive", &cmem, &clen, NULL, 0);
- return *cmem;
+ je_mallctl("stats.active", &cmem, &clen, NULL, 0);
+ return cmem;
#else
return get_max_rss();
#endif
--
2.20.1
