Merge pull request #232 from LLNL/bugfix/pool-release

Fix bug where DynamicPool was leaking blocks (v1.0.1)

CHANGELOG.md

@@ -16,6 +16,13 @@ Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to
 
 ### Fixed
 
+## [1.0.1] - 2019-09-04
+
+### Fixed
+
+- Fixed a bug in DynamicPool where memory could be leaked when allocating a new
+  block using the "minimum size" for an allocation smaller than the block.
+
 ## [1.0.0] - 2019-07-12
 
 ### Added

CMakeLists.txt

@@ -11,7 +11,7 @@ cmake_policy(SET CMP0057 NEW)
 
 project(Umpire
   LANGUAGES CXX C
-  VERSION 1.0.0)
+  VERSION 1.0.1)
 
 set(UMPIRE_VERSION_RC "")
 

RELEASE_NOTES.md

@@ -1,3 +1,8 @@
+# v1.0.1
+
+- Fixed a bug in DynamicPool where memory could be leaked when allocating a new
+  block using the "minimum size" for an allocation smaller than the block.
+
 # v1.0.0
 
 - Umpire is MPI-aware (outputs rank information to logs and replays) when

docs/sphinx/conf.py

@@ -98,7 +98,7 @@
 # The short X.Y version.
 version = u'1.0'
 # The full version, including alpha/beta/rc tags.
-release = u'1.0.0'
+release = u'1.0.1'
 
 # The language for content autogenerated by Sphinx. Refer to documentation
 # for a list of supported languages.

docs/sphinx/conf.py.in

@@ -96,9 +96,9 @@ author = u'David Beckingsale'
 # built documents.
 #
 # The short X.Y version.
-version = u'0.3'
+version = u'1.0'
 # The full version, including alpha/beta/rc tags.
-release = u'0.3.3'
+release = u'1.0.1'
 
 # The language for content autogenerated by Sphinx. Refer to documentation
 # for a list of supported languages.

scripts/make_release_tarball.sh

@@ -7,7 +7,7 @@
 ##############################################################################
 
 TAR_CMD=gtar
-VERSION=1.0.0
+VERSION=1.0.1
 
 git archive --prefix=umpire-${VERSION}/ -o umpire-${VERSION}.tar HEAD 2> /dev/null
 

src/umpire/strategy/DynamicPool.cpp

@@ -33,17 +33,17 @@ DynamicPool::DynamicPool(const std::string& name,
                          const int align_bytes) noexcept :
   AllocationStrategy(name, id),
   m_allocator{allocator.getAllocationStrategy()},
-  m_min_alloc_bytes{min_alloc_bytes},
+  m_min_alloc_bytes{round_up(min_alloc_bytes, align_bytes)},
   m_align_bytes{align_bytes},
   m_coalesce_heuristic{coalesce_heuristic},
   m_used_map{},
   m_free_map{},
   m_curr_bytes{0},
-  m_actual_bytes{initial_alloc_bytes},
+  m_actual_bytes{round_up(initial_alloc_bytes, align_bytes)},
   m_highwatermark{0}
 {
-  insertFree(m_allocator->allocate(initial_alloc_bytes),
-             initial_alloc_bytes, true, initial_alloc_bytes);
+  const std::size_t bytes{round_up(initial_alloc_bytes, align_bytes)};
+  insertFree(m_allocator->allocate(bytes), bytes, true, bytes);
 }
 
 DynamicPool::~DynamicPool()
@@ -180,10 +180,9 @@ void* DynamicPool::allocate(std::size_t bytes)
 
     m_curr_bytes += rounded_bytes;
 
-    const int64_t left_bytes{static_cast<int64_t>(
-        free_size - rounded_bytes)};
+    const std::size_t left_bytes{free_size - rounded_bytes};
 
-    if (left_bytes > m_align_bytes) {
+    if (left_bytes > 0) {
       insertFree(static_cast<unsigned char*>(ptr) + rounded_bytes, left_bytes,
                  false, whole_bytes);
     }
@@ -197,11 +196,10 @@ void* DynamicPool::allocate(std::size_t bytes)
     m_actual_bytes += alloc_bytes;
     m_curr_bytes += rounded_bytes;
 
-    const int64_t left_bytes{static_cast<int64_t>(
-        m_min_alloc_bytes - rounded_bytes)};
+    const std::size_t left_bytes{alloc_bytes - rounded_bytes};
 
     // Add free
-    if (left_bytes > m_align_bytes)
+    if (left_bytes > 0)
       insertFree(static_cast<unsigned char*>(ptr) + rounded_bytes, left_bytes,
                  false, alloc_bytes);
   }

tests/integration/strategy_tests.cpp

@@ -389,11 +389,43 @@ TEST(ReleaseTest, Works)
   void* ptr_two = alloc.allocate(1024);
   alloc.deallocate(ptr_two);
 
+  ASSERT_EQ(alloc.getCurrentSize(), 62);
   EXPECT_NO_THROW(alloc.release());
 
+  ASSERT_LE(alloc.getActualSize(), 64);
+
   alloc.deallocate(ptr_one);
+  ASSERT_EQ(alloc.getCurrentSize(), 0);
+
+  EXPECT_NO_THROW(alloc.release());
+
+  ASSERT_EQ(alloc.getCurrentSize(), 0);
+  ASSERT_LE(alloc.getActualSize(), 0);
 }
 
+TEST(ReleaseTest, MissingBlocks)
+{
+  auto& rm = umpire::ResourceManager::getInstance();
+
+  auto allocator = rm.makeAllocator<umpire::strategy::DynamicPool>(
+      "host_dyn_pool_for_release_2", rm.getAllocator("HOST"), 128, 64);
+
+  void* data_one = allocator.allocate(128);
+  void* data_two = allocator.allocate(44);
+
+  allocator.deallocate(data_one);
+  allocator.deallocate(data_two);
+
+  ASSERT_EQ(allocator.getCurrentSize(), 0);
+  ASSERT_GE(allocator.getActualSize(), 0);
+
+  allocator.release();
+
+  ASSERT_EQ(allocator.getCurrentSize(), 0);
+  ASSERT_EQ(allocator.getActualSize(), 0);
+}
+
+
 TEST(DynamicPool, coalesce)
 {
   auto& rm = umpire::ResourceManager::getInstance();

tools/util/ReplayInterpreter.cpp

@@ -113,12 +113,21 @@ void ReplayInterpreter::buildOperations(void)
           || m_json["payload"]["minor"] != UMPIRE_VERSION_MINOR
           || m_json["payload"]["patch"] != UMPIRE_VERSION_PATCH ) {
 
-        REPLAY_ERROR("Warning, version mismatch:\n"
+        REPLAY_WARNING("Warning, version mismatch:\n"
           << "  Tool version: " << UMPIRE_VERSION_MAJOR << "." << UMPIRE_VERSION_MINOR << "." << UMPIRE_VERSION_PATCH << std::endl
           << "  Log  version: "
           << m_json["payload"]["major"] << "."
           << m_json["payload"]["minor"]  << "."
           << m_json["payload"]["patch"]);
+
+        if (m_json["payload"]["major"] != UMPIRE_VERSION_MAJOR) {
+          REPLAY_ERROR("Warning, major version mismatch:\n"
+            << "  Tool version: " << UMPIRE_VERSION_MAJOR << "." << UMPIRE_VERSION_MINOR << "." << UMPIRE_VERSION_PATCH << std::endl
+            << "  Log  version: "
+            << m_json["payload"]["major"] << "."
+            << m_json["payload"]["minor"]  << "."
+            << m_json["payload"]["patch"]);
+        }
       }
     }
     else {
@@ -178,12 +187,21 @@ int ReplayInterpreter::getSymbolicOperation( std::string& raw_line, std::string&
           || m_json["payload"]["minor"] != UMPIRE_VERSION_MINOR
           || m_json["payload"]["patch"] != UMPIRE_VERSION_PATCH ) {
 
-        REPLAY_ERROR("Warning, version mismatch:\n"
+        REPLAY_WARNING("Warning, version mismatch:\n"
           << "  Tool version: " << UMPIRE_VERSION_MAJOR << "." << UMPIRE_VERSION_MINOR << "." << UMPIRE_VERSION_PATCH << std::endl
           << "  Log  version: "
           << m_json["payload"]["major"] << "."
           << m_json["payload"]["minor"]  << "."
           << m_json["payload"]["patch"]);
+
+        if (m_json["payload"]["major"] != UMPIRE_VERSION_MAJOR) {
+          REPLAY_ERROR("Warning, major version mismatch:\n"
+            << "  Tool version: " << UMPIRE_VERSION_MAJOR << "." << UMPIRE_VERSION_MINOR << "." << UMPIRE_VERSION_PATCH << std::endl
+            << "  Log  version: "
+            << m_json["payload"]["major"] << "."
+            << m_json["payload"]["minor"]  << "."
+            << m_json["payload"]["patch"]);
+        }
       }
     }
     else {

tools/util/ReplayMacros.hpp

@@ -10,6 +10,12 @@
 #include <cstdlib>
 #include <iostream>
 
+#define REPLAY_WARNING( msg )                                               \
+{                                                                           \
+  std::cout << std::string(__FILE__) << " " << __LINE__ << " " << __func__  \
+    << " " << msg << std::endl;                                             \
+}       
+
 #define REPLAY_ERROR( msg )                                                 \
 {                                                                           \
   std::cerr << std::string(__FILE__) << " " << __LINE__ << " " << __func__  \