pitrou commented on code in PR #13516:
URL: https://github.com/apache/arrow/pull/13516#discussion_r976141800
##########
cpp/src/arrow/memory_pool_test.cc:
##########
@@ -168,7 +169,103 @@ TEST(Jemalloc, SetDirtyPageDecayMillis) {
#ifdef ARROW_JEMALLOC
ASSERT_OK(jemalloc_set_decay_ms(0));
#else
- ASSERT_RAISES(Invalid, jemalloc_set_decay_ms(0));
+ ASSERT_RAISES(NotImplemented, jemalloc_set_decay_ms(0));
+#endif
+}
+
+TEST(Jemalloc, GetAllocationStats) {
+#ifdef ARROW_JEMALLOC
+ uint8_t* data;
+ int64_t allocated, active, metadata, resident, mapped, retained, allocated0,
active0,
+ metadata0, resident0, mapped0, retained0;
+ int64_t thread_allocated, thread_deallocated, thread_peak_read,
thread_allocated0,
+ thread_deallocated0, thread_peak_read0;
+ auto pool = default_memory_pool();
Review Comment:
Nit, but why not leave it null here? It's initialized on the following line.
```suggestion
MemoryPool* pool = nullptr;
```
##########
cpp/src/arrow/memory_pool_test.cc:
##########
@@ -168,7 +169,103 @@ TEST(Jemalloc, SetDirtyPageDecayMillis) {
#ifdef ARROW_JEMALLOC
ASSERT_OK(jemalloc_set_decay_ms(0));
#else
- ASSERT_RAISES(Invalid, jemalloc_set_decay_ms(0));
+ ASSERT_RAISES(NotImplemented, jemalloc_set_decay_ms(0));
+#endif
+}
+
+TEST(Jemalloc, GetAllocationStats) {
+#ifdef ARROW_JEMALLOC
+ uint8_t* data;
+ int64_t allocated, active, metadata, resident, mapped, retained, allocated0,
active0,
+ metadata0, resident0, mapped0, retained0;
+ int64_t thread_allocated, thread_deallocated, thread_peak_read,
thread_allocated0,
+ thread_deallocated0, thread_peak_read0;
+ auto pool = default_memory_pool();
+ ABORT_NOT_OK(jemalloc_memory_pool(&pool));
+ ASSERT_EQ("jemalloc", pool->backend_name());
+
+ // Record stats before allocating
+ ASSERT_OK_AND_ASSIGN(allocated0, jemalloc_get_stat("stats.allocated"));
+ ASSERT_OK_AND_ASSIGN(active0, jemalloc_get_stat("stats.active"));
+ ASSERT_OK_AND_ASSIGN(metadata0, jemalloc_get_stat("stats.metadata"));
+ ASSERT_OK_AND_ASSIGN(resident0, jemalloc_get_stat("stats.resident"));
+ ASSERT_OK_AND_ASSIGN(mapped0, jemalloc_get_stat("stats.mapped"));
+ ASSERT_OK_AND_ASSIGN(retained0, jemalloc_get_stat("stats.retained"));
+ ASSERT_OK_AND_ASSIGN(thread_allocated0,
jemalloc_get_stat("thread.allocated"));
+ ASSERT_OK_AND_ASSIGN(thread_deallocated0,
jemalloc_get_stat("thread.deallocated"));
+ ASSERT_OK_AND_ASSIGN(thread_peak_read0,
jemalloc_get_stat("thread.peak.read"));
+
+ // Allocate memory
+ ASSERT_OK(jemalloc_set_decay_ms(10000));
Review Comment:
Why this? If this test depends on timing it may fail on some slow CI
configurations...
##########
cpp/src/arrow/memory_pool_jemalloc.cc:
##########
@@ -153,4 +154,59 @@ Status jemalloc_set_decay_ms(int ms) {
#undef RETURN_IF_JEMALLOC_ERROR
+#ifdef ARROW_JEMALLOC
Review Comment:
I don't think the ifdef is necessary? This file will only be compiled if
jemalloc is enabled.
##########
cpp/src/arrow/memory_pool_jemalloc.cc:
##########
@@ -153,4 +154,59 @@ Status jemalloc_set_decay_ms(int ms) {
#undef RETURN_IF_JEMALLOC_ERROR
+#ifdef ARROW_JEMALLOC
+Result<int64_t> jemalloc_get_stat(const char* name) {
+ size_t sz = sizeof(uint64_t);
+ int err;
+ uint64_t value;
+
+ // Update the statistics cached by mallctl.
+ if (std::strcmp(name, "stats.allocated") == 0 ||
+ std::strcmp(name, "stats.active") == 0 ||
+ std::strcmp(name, "stats.metadata") == 0 ||
+ std::strcmp(name, "stats.resident") == 0 ||
+ std::strcmp(name, "stats.mapped") == 0 ||
+ std::strcmp(name, "stats.retained") == 0) {
+ uint64_t epoch;
+ mallctl("epoch", &epoch, &sz, &epoch, sz);
+ }
+
+ err = mallctl(name, &value, &sz, nullptr, 0);
+
+ if (err) {
+ return arrow::internal::IOErrorFromErrno(err, "Failed retrieving ", &name);
+ }
+
+ return value;
+}
+
+Status jemalloc_peak_reset() {
+ int err = mallctl("thread.peak.reset", nullptr, nullptr, nullptr, 0);
+ return err ? arrow::internal::IOErrorFromErrno(err, "Failed resetting
thread.peak.")
+ : Status::OK();
+}
+
+Result<std::string> jemalloc_stats_string(const char* opts) {
+ std::string stats;
+ auto write_cb = [&stats](const char* str) { stats.append(str); };
+ ARROW_UNUSED(jemalloc_stats_print(write_cb, opts));
+ return stats;
+}
+
+Status jemalloc_stats_print(const char* opts) {
+ malloc_stats_print(nullptr, nullptr, opts);
+ return Status::OK();
+}
+
+Status jemalloc_stats_print(std::function<void(const char*)> write_cb, const
char* opts) {
+ auto cb_wrapper = [](void* opaque, const char* str) {
+ (*static_cast<std::function<void(const char*)>*>(opaque))(str);
+ };
+ if (write_cb) {
Review Comment:
Is this required? I think the caller should make sure that `write_cb` is
initialized.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
