Fix memleak, reported by @BearishSun in PR #103

README.md

@@ -35,7 +35,7 @@
 1. Create `conanfile.txt` in your source dir (don't forget to update the version)
    ```ini
    [requires]
-   robin-hood-hashing/3.9.0
+   robin-hood-hashing/3.9.1
 
    [generators]
    cmake

src/include/robin_hood.h

@@ -36,7 +36,7 @@
 // see https://semver.org/
 #define ROBIN_HOOD_VERSION_MAJOR 3 // for incompatible API changes
 #define ROBIN_HOOD_VERSION_MINOR 9 // for adding functionality in a backwards-compatible manner
-#define ROBIN_HOOD_VERSION_PATCH 0 // for backwards-compatible bug fixes
+#define ROBIN_HOOD_VERSION_PATCH 1 // for backwards-compatible bug fixes
 
 #include <algorithm>
 #include <cstdlib>
@@ -54,16 +54,17 @@
 // #define ROBIN_HOOD_LOG_ENABLED
 #ifdef ROBIN_HOOD_LOG_ENABLED
 #    include <iostream>
-#    define ROBIN_HOOD_LOG(x) std::cout << __FUNCTION__ << "@" << __LINE__ << ": " << x << std::endl
+#    define ROBIN_HOOD_LOG(...) \
+        std::cout << __FUNCTION__ << "@" << __LINE__ << ": " << __VA_ARGS__ << std::endl
 #else
 #    define ROBIN_HOOD_LOG(x)
 #endif
 
 // #define ROBIN_HOOD_TRACE_ENABLED
 #ifdef ROBIN_HOOD_TRACE_ENABLED
 #    include <iostream>
-#    define ROBIN_HOOD_TRACE(x) \
-        std::cout << __FUNCTION__ << "@" << __LINE__ << ": " << x << std::endl
+#    define ROBIN_HOOD_TRACE(...) \
+        std::cout << __FUNCTION__ << "@" << __LINE__ << ": " << __VA_ARGS__ << std::endl
 #else
 #    define ROBIN_HOOD_TRACE(x)
 #endif
@@ -391,6 +392,7 @@ class BulkPoolAllocator {
     void reset() noexcept {
         while (mListForFree) {
             T* tmp = *mListForFree;
+            ROBIN_HOOD_LOG("std::free");
             std::free(mListForFree);
             mListForFree = reinterpret_cast_no_cast_align_warning<T**>(tmp);
         }
@@ -426,8 +428,10 @@ class BulkPoolAllocator {
         // calculate number of available elements in ptr
         if (numBytes < ALIGNMENT + ALIGNED_SIZE) {
             // not enough data for at least one element. Free and return.
+            ROBIN_HOOD_LOG("std::free");
             std::free(ptr);
         } else {
+            ROBIN_HOOD_LOG("add to buffer");
             add(ptr, numBytes);
         }
     }
@@ -491,8 +495,9 @@ class BulkPoolAllocator {
         size_t const numElementsToAlloc = calcNumElementsToAlloc();
 
         // alloc new memory: [prev |T, T, ... T]
-        // std::cout << (sizeof(T*) + ALIGNED_SIZE * numElementsToAlloc) << " bytes" << std::endl;
         size_t const bytes = ALIGNMENT + ALIGNED_SIZE * numElementsToAlloc;
+        ROBIN_HOOD_LOG("std::malloc " << bytes << " = " << ALIGNMENT << " + " << ALIGNED_SIZE
+                                      << " * " << numElementsToAlloc);
         add(assertNotNull<std::bad_alloc>(std::malloc(bytes)), bytes);
         return mHead;
     }
@@ -529,6 +534,7 @@ struct NodeAllocator<T, MinSize, MaxSize, true> {
 
     // we are not using the data, so just free it.
     void addOrFree(void* ptr, size_t ROBIN_HOOD_UNUSED(numBytes) /*unused*/) noexcept {
+        ROBIN_HOOD_LOG("std::free");
         std::free(ptr);
     }
 };
@@ -1571,8 +1577,12 @@ class Table
             // elements and insert them, but copying is probably faster.
 
             auto const numElementsWithBuffer = calcNumElementsWithBuffer(o.mMask + 1);
-            mKeyVals = static_cast<Node*>(detail::assertNotNull<std::bad_alloc>(
-                std::malloc(calcNumBytesTotal(numElementsWithBuffer))));
+            auto const numBytesTotal = calcNumBytesTotal(numElementsWithBuffer);
+
+            ROBIN_HOOD_LOG("std::malloc " << numBytesTotal << " = calcNumBytesTotal("
+                                          << numElementsWithBuffer << ")");
+            mKeyVals = static_cast<Node*>(
+                detail::assertNotNull<std::bad_alloc>(std::malloc(numBytesTotal)));
             // no need for calloc because clonData does memcpy
             mInfo = reinterpret_cast<uint8_t*>(mKeyVals + numElementsWithBuffer);
             mNumElements = o.mNumElements;
@@ -1620,12 +1630,16 @@ class Table
             // no luck: we don't have the same array size allocated, so we need to realloc.
             if (0 != mMask) {
                 // only deallocate if we actually have data!
+                ROBIN_HOOD_LOG("std::free");
                 std::free(mKeyVals);
             }
 
             auto const numElementsWithBuffer = calcNumElementsWithBuffer(o.mMask + 1);
-            mKeyVals = static_cast<Node*>(detail::assertNotNull<std::bad_alloc>(
-                std::malloc(calcNumBytesTotal(numElementsWithBuffer))));
+            auto const numBytesTotal = calcNumBytesTotal(numElementsWithBuffer);
+            ROBIN_HOOD_LOG("std::malloc " << numBytesTotal << " = calcNumBytesTotal("
+                                          << numElementsWithBuffer << ")");
+            mKeyVals = static_cast<Node*>(
+                detail::assertNotNull<std::bad_alloc>(std::malloc(numBytesTotal)));
 
             // no need for calloc here because cloneData performs a memcpy.
             mInfo = reinterpret_cast<uint8_t*>(mKeyVals + numElementsWithBuffer);
@@ -1967,23 +1981,15 @@ class Table
     // reserves space for the specified number of elements. Makes sure the old data fits.
     // exactly the same as reserve(c).
     void rehash(size_t c) {
-        reserve(c);
+        // forces a reserve
+        reserve(c, true);
     }
 
     // reserves space for the specified number of elements. Makes sure the old data fits.
-    // Exactly the same as resize(c). Use resize(0) to shrink to fit.
+    // Exactly the same as rehash(c). Use rehash(0) to shrink to fit.
     void reserve(size_t c) {
-        ROBIN_HOOD_TRACE(this)
-        auto const minElementsAllowed = (std::max)(c, mNumElements);
-        auto newSize = InitialNumElements;
-        while (calcMaxNumElementsAllowed(newSize) < minElementsAllowed && newSize != 0) {
-            newSize *= 2;
-        }
-        if (ROBIN_HOOD_UNLIKELY(newSize == 0)) {
-            throwOverflowError();
-        }
-
-        rehashPowerOfTwo(newSize);
+        // reserve, but don't force rehash
+        reserve(c, false);
     }
 
     size_type size() const noexcept { // NOLINT(modernize-use-nodiscard)
@@ -2074,6 +2080,26 @@ class Table
         return find(e) != end();
     }
 
+    void reserve(size_t c, bool forceRehash) {
+        ROBIN_HOOD_TRACE(this)
+        auto const minElementsAllowed = (std::max)(c, mNumElements);
+        auto newSize = InitialNumElements;
+        while (calcMaxNumElementsAllowed(newSize) < minElementsAllowed && newSize != 0) {
+            newSize *= 2;
+        }
+        if (ROBIN_HOOD_UNLIKELY(newSize == 0)) {
+            throwOverflowError();
+        }
+
+        ROBIN_HOOD_LOG("newSize > mMask + 1: " << newSize << " > " << mMask << " + 1");
+
+        // only actually do anything when the new size is bigger than the old one. This prevents to
+        // continuously allocate for each reserve() call.
+        if (forceRehash || newSize > mMask + 1) {
+            rehashPowerOfTwo(newSize);
+        }
+    }
+
     // reserves space for at least the specified number of elements.
     // only works if numBuckets if power of two
     void rehashPowerOfTwo(size_t numBuckets) {
@@ -2144,8 +2170,11 @@ class Table
         auto const numElementsWithBuffer = calcNumElementsWithBuffer(max_elements);
 
         // calloc also zeroes everything
-        mKeyVals = reinterpret_cast<Node*>(detail::assertNotNull<std::bad_alloc>(
-            std::calloc(1, calcNumBytesTotal(numElementsWithBuffer))));
+        auto const numBytesTotal = calcNumBytesTotal(numElementsWithBuffer);
+        ROBIN_HOOD_LOG("std::calloc " << numBytesTotal << " = calcNumBytesTotal("
+                                      << numElementsWithBuffer << ")");
+        mKeyVals = reinterpret_cast<Node*>(
+            detail::assertNotNull<std::bad_alloc>(std::calloc(1, numBytesTotal)));
         mInfo = reinterpret_cast<uint8_t*>(mKeyVals + numElementsWithBuffer);
 
         // set sentinel
@@ -2269,7 +2298,7 @@ class Table
     bool try_increase_info() {
         ROBIN_HOOD_LOG("mInfoInc=" << mInfoInc << ", numElements=" << mNumElements
                                    << ", maxNumElementsAllowed="
-                                   << calcMaxNumElementsAllowed(mMask + 1))
+                                   << calcMaxNumElementsAllowed(mMask + 1));
         if (mInfoInc <= 2) {
             // need to be > 2 so that shift works (otherwise undefined behavior!)
             return false;
@@ -2309,7 +2338,7 @@ class Table
         ROBIN_HOOD_LOG("mNumElements=" << mNumElements << ", maxNumElementsAllowed="
                                        << maxNumElementsAllowed << ", load="
                                        << (static_cast<double>(mNumElements) * 100.0 /
-                                           (static_cast<double>(mMask) + 1)))
+                                           (static_cast<double>(mMask) + 1)));
         // it seems we have a really bad hash function! don't try to resize again
         if (mNumElements * 2 < calcMaxNumElementsAllowed(mMask + 1)) {
             throwOverflowError();
@@ -2332,6 +2361,7 @@ class Table
         // reports a compile error: attempt to free a non-heap object 'fm'
         // [-Werror=free-nonheap-object]
         if (mKeyVals != reinterpret_cast_no_cast_align_warning<Node*>(&mMask)) {
+            ROBIN_HOOD_LOG("std::free");
             std::free(mKeyVals);
         }
     }

src/test/unit/CMakeLists.txt

@@ -63,6 +63,7 @@ target_sources_local(rh PRIVATE
     unit_iterators_stochastic.cpp
     unit_load_factor.cpp
     unit_maps_of_maps.cpp
+    unit_memleak_reserve.cpp
     unit_multiple_apis.cpp
     unit_mup.cpp
     unit_no_intrinsics.cpp

src/test/unit/unit_memleak_reserve.cpp

@@ -0,0 +1,19 @@
+#include <robin_hood.h>
+
+#include <app/doctest.h>
+
+#include <vector>
+
+TEST_CASE("memleak_reserve_flat") {
+    robin_hood::unordered_flat_map<int, int> m;
+    for (size_t i = 0; i < 10; ++i) {
+        m.reserve(1000);
+    }
+}
+
+TEST_CASE("memleak_reserve_node") {
+    robin_hood::unordered_node_map<int, int> m;
+    for (size_t i = 0; i < 10; ++i) {
+        m.reserve(1000);
+    }
+}

src/test/unit/unit_reserve.cpp

@@ -21,8 +21,12 @@ TEST_CASE_TEMPLATE("reserve", Map, robin_hood::unordered_flat_map<uint64_t, uint
 
     REQUIRE(2047 == map.mask());
 
-    // shrink map to best fit
+    // reserve(0) does *not* shrink map
     map.reserve(0);
+    REQUIRE(2047 == map.mask());
+
+    // rehash(0) *does* shrink the map
+    map.rehash(0);
     REQUIRE(127 == map.mask());
 }