[GitHub] [arrow] pitrou commented on a diff in pull request #13516: ARROW-16981: [C++] Expose jemalloc statistics for logging

[GitBox <gi...@apache.org>]

pitrou commented on code in PR #13516:
URL: https://github.com/apache/arrow/pull/13516#discussion_r917935181


##########
cpp/src/arrow/memory_pool_jemalloc.cc:
##########
@@ -151,6 +151,26 @@ Status jemalloc_set_decay_ms(int ms) {
   return Status::OK();
 }
 
+Status jemalloc_mallctl(const char* name, void* oldp, size_t* oldlenp, void* newp,
+                        size_t newlen) {
+#ifdef ARROW_JEMALLOC
+  int res = mallctl(name, oldp, oldlenp, newp, newlen);
+  return res ? Status::UnknownError(res) : Status::OK();

Review Comment:
   Better, use `IOErrorFromErrno`.



##########
cpp/src/arrow/memory_pool.h:
##########
@@ -180,6 +180,12 @@ Status jemalloc_set_decay_ms(int ms);
 /// May return NotImplemented if mimalloc is not available.
 ARROW_EXPORT Status mimalloc_memory_pool(MemoryPool** out);
 
+ARROW_EXPORT Status jemalloc_mallctl(const char* name, void* oldp, size_t* oldlenp,

Review Comment:
   For example:
   ```c++
   Status jemalloc_get_stat(const char* name, bool* out);
   Status jemalloc_get_stat(const char* name, int64_t* out);
   Status jemalloc_get_stat(const char* name, uint64_t* out);
   Status jemalloc_get_stat(const char* name, const char** out);
   ```
   (passing the result as out-parameter to allow for several signatures; we could also return a variant but that would add a heavy header inclusion)



##########
cpp/src/arrow/memory_pool.h:
##########
@@ -180,6 +180,12 @@ Status jemalloc_set_decay_ms(int ms);
 /// May return NotImplemented if mimalloc is not available.
 ARROW_EXPORT Status mimalloc_memory_pool(MemoryPool** out);
 
+ARROW_EXPORT Status jemalloc_mallctl(const char* name, void* oldp, size_t* oldlenp,

Review Comment:
   I'm not sure, but I would favour exposing higher-level APIs rather than this raw C ugliness.



##########
cpp/src/arrow/memory_pool.h:
##########
@@ -180,6 +180,12 @@ Status jemalloc_set_decay_ms(int ms);
 /// May return NotImplemented if mimalloc is not available.
 ARROW_EXPORT Status mimalloc_memory_pool(MemoryPool** out);
 
+ARROW_EXPORT Status jemalloc_mallctl(const char* name, void* oldp, size_t* oldlenp,
+                                     void* newp, size_t newlen);
+
+ARROW_EXPORT Status jemalloc_stats_print(void (*write_cb)(void*, const char*),
+                                         void* cbopaque, const char* opts);

Review Comment:
   Wow, can this take a `std::function` instead?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org