Fixed review comments.

Author: k0ushal

core/resource_manager.hpp

@@ -74,6 +74,8 @@ struct ResourceManagementOptions {
   IResourceManager* cached_columns{&IResourceManager::kNoop};
 };
 
+//  Memory manager for IResearch.
+//  This is a singleton class
 struct IResearchMemoryManager : public IResourceManager {
 protected:
   IResearchMemoryManager() = default;
@@ -86,19 +88,19 @@ struct IResearchMemoryManager : public IResourceManager {
     IRS_ASSERT(this != &kForbidden);
     IRS_ASSERT(value >= 0);
 
-    if (0 == IResearchMemoryManager::_memory_limit) {
+    if (0 == _memoryLimit) {
       // since we have no limit, we can simply use fetch-add for the increment
-      IResearchMemoryManager::_current.fetch_add(value, std::memory_order_relaxed);
+      _current.fetch_add(value, std::memory_order_relaxed);
     } else {
       // we only want to perform the update if we don't exceed the limit!
-      std::uint64_t cur = IResearchMemoryManager::_current.load(std::memory_order_relaxed);
+      std::uint64_t cur = _current.load(std::memory_order_relaxed);
       std::uint64_t next;
       do {
         next = cur + value;
-        if (IRS_UNLIKELY(next > IResearchMemoryManager::_memory_limit.load(std::memory_order_relaxed))) {
+        if (IRS_UNLIKELY(next > _memoryLimit.load(std::memory_order_relaxed))) {
           throw std::bad_alloc();
         }
-      } while (!IResearchMemoryManager::_current.compare_exchange_weak(
+      } while (!_current.compare_exchange_weak(
         cur, next, std::memory_order_relaxed));
     }
   }
@@ -109,28 +111,30 @@ struct IResearchMemoryManager : public IResourceManager {
     _current.fetch_sub(value, std::memory_order_relaxed);
   }
 
-protected:
-  //  This limit can be set only by IResearchFeature.
-  //  During IResearchFeature::prepare() it is set to a pre-defined
-  //  percentage of the total available physical memory or to the value
-  //  of ARANGODB_OVERRIDE_DETECTED_TOTAL_MEMORY option if specified.
-  static inline std::atomic<std::uint64_t> _memory_limit = { 0 };
-
-  //  Singleton
-  static inline std::shared_ptr<IResourceManager> _instance;
+  //  NOTE: IResearchFeature owns and manages this memory limit.
+  //  That is why this method should only be used by IResearchFeature.
+  virtual void SetMemoryLimit(uint64_t memoryLimit) {
+    _memoryLimit.store(memoryLimit);
+  }
 
 private:
-  static inline std::atomic<std::uint64_t> _current = { 0 };
+  //  This limit should be set exclusively by IResearchFeature.
+  //  During IResearchFeature::validateOptions() this limit is set to a
+  //  percentage of either the total available physical memory or the value
+  //  of ARANGODB_OVERRIDE_DETECTED_TOTAL_MEMORY envvar if specified.
+  std::atomic<std::uint64_t> _memoryLimit = { 0 };
+  std::atomic<std::uint64_t> _current = { 0 };
+
+  //  Singleton
+  static inline std::shared_ptr<IResearchMemoryManager> _instance;
 
 public:
-  static std::shared_ptr<IResourceManager> GetInstance() {
+  static std::shared_ptr<IResearchMemoryManager> GetInstance() {
     if (!_instance.get())
       _instance.reset(new IResearchMemoryManager());
 
     return _instance;
   }
-
-  friend class arangodb::iresearch::IResearchFeature;
 };
 
 template<typename T>

tests/memory/IResearchMemoryManager_tests.cpp

@@ -29,72 +29,24 @@
 
 using namespace irs;
 
-//  Wrapper class around IResearchMemoryManager.
-//  We use it to track memory increase/decrease.
-//  To be used mostly for debugging purposes.
-struct IResearchMemoryManagerAccessor : public irs::IResearchMemoryManager {
+TEST(IResearchMemoryLimitTest, managed_allocator_smoke_test) {
 
-    IResearchMemoryManagerAccessor() = default;
-    virtual ~IResearchMemoryManagerAccessor() = default;
-
-    virtual void Increase([[maybe_unused]] uint64_t value) override {
-
-        // std::cout << "Increase: " << value << std::endl;
-        IResearchMemoryManager::Increase(value);
-    }
-
-    virtual void Decrease([[maybe_unused]] uint64_t value) noexcept override {
-        // std::cout << "Decrease: " << value << std::endl;
-        IResearchMemoryManager::Decrease(value);
-    }
-
-    static std::shared_ptr<IResearchMemoryManagerAccessor> GetInstance() {
-        if (!_accessor)
-            _accessor.reset(new IResearchMemoryManagerAccessor());
-
-        return _accessor;
-    }
-
-    static void setMemoryLimit(uint64_t limit) {
-      irs::IResearchMemoryManager::_memory_limit.store(limit);
-    }
-
-private:
-    static inline std::shared_ptr<IResearchMemoryManagerAccessor> _accessor;
-};
-
-//  An allocator using IResearchMemoryManagerAccessor
-//  so that we can track the allocations/deallocations
-//  during the test.
-template<typename T>
-struct ManagedTypedAllocatorTest
-  : ManagedAllocator<std::allocator<T>, IResearchMemoryManagerAccessor> {
-  using Base = ManagedAllocator<std::allocator<T>, IResearchMemoryManagerAccessor>;
-  explicit ManagedTypedAllocatorTest()
-    : Base(
-        *IResearchMemoryManagerAccessor::GetInstance()
-      ) {
-  }
-  using Base::Base;
-};
-
-TEST(IResearch_memory_limit_test, managed_allocator_smoke_test) {
-
-    IResearchMemoryManagerAccessor::setMemoryLimit(3);
-    auto memoryMgr = IResearchMemoryManagerAccessor::GetInstance();
+    auto memoryMgr = IResearchMemoryManager::GetInstance();
+    memoryMgr->SetMemoryLimit(3);
 
     using type = char;
-    std::vector<type, ManagedTypedAllocatorTest<type>> arr(*memoryMgr.get());
+    std::vector<type, ManagedTypedAllocator<type>> arr(*memoryMgr);
 
     arr.push_back('a');
     arr.push_back('b');
     ASSERT_THROW(arr.push_back('c'), std::bad_alloc);
 }
 
-TEST(IResearch_memory_limit_test, memory_manager_smoke_test) {
+TEST(IResearchMemoryLimitTest, memory_manager_smoke_test) {
 
     //  set limit
-    IResearchMemoryManagerAccessor::setMemoryLimit(47);
+    auto memoryMgr = IResearchMemoryManager::GetInstance();
+    memoryMgr->SetMemoryLimit(47);
 
     //  allocate vector
     ManagedVector<uint64_t> vec;
@@ -104,7 +56,7 @@ TEST(IResearch_memory_limit_test, memory_manager_smoke_test) {
     ASSERT_THROW(vec.push_back(12), std::bad_alloc);    //  Allocate 32 while holding previous 16, total 48 (bad_alloc)
 
     //  Increase memory limit to accommodate the 3rd element.
-    IResearchMemoryManagerAccessor::setMemoryLimit(48);
+    memoryMgr->SetMemoryLimit(48);
 
     ASSERT_NO_THROW(vec.push_back(12));
 }