From 241766f2ca8158c18eeecc226ce5714217e90325 Mon Sep 17 00:00:00 2001
From: Larry Ruane <larryruane@gmail.com>
Date: Mon, 25 Sep 2023 14:14:39 -0600
Subject: [PATCH] improve MallocUsage() accuracy

---
 src/Makefile.test.include           |  1 +
 src/memusage.h                      | 17 ++++------
 src/test/malloc_usage_tests.cpp     | 51 +++++++++++++++++++++++++++++
 src/test/validation_flush_tests.cpp |  2 +-
 4 files changed, 59 insertions(+), 12 deletions(-)
 create mode 100644 src/test/malloc_usage_tests.cpp

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index 5dc20d4fab9fa..4f80c9deba7a7 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -103,6 +103,7 @@ BITCOIN_TESTS =\
   test/key_io_tests.cpp \
   test/key_tests.cpp \
   test/logging_tests.cpp \
+  test/malloc_usage_tests.cpp \
   test/mempool_tests.cpp \
   test/merkle_tests.cpp \
   test/merkleblock_tests.cpp \
diff --git a/src/memusage.h b/src/memusage.h
index 08be66172e361..15e49aeeb9f0b 100644
--- a/src/memusage.h
+++ b/src/memusage.h
@@ -50,16 +50,8 @@ template<typename X> static inline size_t DynamicUsage(const X * const &v) { ret
 
 static inline size_t MallocUsage(size_t alloc)
 {
-    // Measured on libc6 2.19 on Linux.
-    if (alloc == 0) {
-        return 0;
-    } else if (sizeof(void*) == 8) {
-        return ((alloc + 31) >> 4) << 4;
-    } else if (sizeof(void*) == 4) {
-        return ((alloc + 15) >> 3) << 3;
-    } else {
-        assert(0);
-    }
+    static_assert(sizeof(void*) == 4 || sizeof(void*) == 8);
+    return (sizeof(void*) == 8) ? std::max<size_t>(32, (alloc + 23) & ~15) : (alloc + 19) & ~15;
 }
 
 // STL data structures
@@ -93,7 +85,10 @@ static inline size_t DynamicUsage(const std::vector<X>& v)
 template<unsigned int N, typename X, typename S, typename D>
 static inline size_t DynamicUsage(const prevector<N, X, S, D>& v)
 {
-    return MallocUsage(v.allocated_memory());
+    // If allocated_memory() returns zero, we haven't done an actual
+    // zero-byte allocation, which does require physical memory.
+    size_t am{v.allocated_memory()};
+    return am > 0 ? MallocUsage(am) : 0;
 }
 
 template<typename X, typename Y>
diff --git a/src/test/malloc_usage_tests.cpp b/src/test/malloc_usage_tests.cpp
new file mode 100644
index 0000000000000..ec1105025462a
--- /dev/null
+++ b/src/test/malloc_usage_tests.cpp
@@ -0,0 +1,51 @@
+// Copyright (c) 2023 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or https://www.opensource.org/licenses/mit-license.php.
+
+#include <memusage.h>
+
+#include <test/util/setup_common.h>
+
+#include <boost/test/unit_test.hpp>
+
+BOOST_FIXTURE_TEST_SUITE(malloc_usage_tests, BasicTestingSetup)
+
+//! Test the accuracy of MallocUsage() by noting how far the returned
+//! pointer advances for each allocation; most of the time it should
+//! be as predicted by MallocUsage(). Do this across various allocation
+//! sizes; it's probably sufficient to not go beyond 256, because any
+//! error on sizes larger than that won't be large as a fraction.
+BOOST_AUTO_TEST_CASE(malloc_usage)
+{
+    std::vector<void*> alloc_pointers;
+    for (size_t size{0}; size <= 256; ++size) {
+        void* prev_p{};
+        const size_t malloc_usage{memusage::MallocUsage(size)};
+        int accurate{0};
+        int inaccurate{0};
+        constexpr int repetitions{10000};
+        for (int i{0}; i < repetitions; ++i) {
+            void* p{malloc(size)};
+            alloc_pointers.push_back(p);
+            intptr_t p_int{intptr_t(p)};
+            intptr_t prev_p_int{intptr_t(prev_p)};
+            // Due to earlier allocations (fragmentation), allocation
+            // addresses can jump around; if the latest one is too
+            // far from the previous one, ignore it.
+            if (p_int > prev_p_int && p_int < prev_p_int + intptr_t(malloc_usage * 4)) {
+                if (size_t(p_int - prev_p_int) == malloc_usage) {
+                    ++accurate;
+                } else {
+                    ++inaccurate;
+                }
+            }
+            prev_p = p;
+        }
+        BOOST_CHECK_GT(accurate, inaccurate * 10);
+    }
+    for (void* p : alloc_pointers) {
+        free(p);
+    }
+}
+
+BOOST_AUTO_TEST_SUITE_END()
\ No newline at end of file
diff --git a/src/test/validation_flush_tests.cpp b/src/test/validation_flush_tests.cpp
index 7398091215cad..667132a39d24d 100644
--- a/src/test/validation_flush_tests.cpp
+++ b/src/test/validation_flush_tests.cpp
@@ -30,7 +30,7 @@ BOOST_AUTO_TEST_CASE(getcoinscachesizestate)
     // (prevector<28, unsigned char>) when assigned 56 bytes of data per above.
     //
     // See also: Coin::DynamicMemoryUsage().
-    constexpr unsigned int COIN_SIZE = is_64_bit ? 80 : 64;
+    constexpr size_t COIN_SIZE{64};
 
     auto print_view_mem_usage = [](CCoinsViewCache& view) {
         BOOST_TEST_MESSAGE("CCoinsViewCache memory usage: " << view.DynamicMemoryUsage());