Merge pull request #24366 from vespa-engine/geirst/datastore-refactor

Refactor Datastore to hide implementation details

5 files changed, 71 insertions, 58 deletions

diff --git a/vespalib/src/tests/datastore/datastore/datastore_test.cpp b/vespalib/src/tests/datastore/datastore/datastore_test.cpp
index 484da33aeb2..2c4e68467da 100644
--- a/vespalib/src/tests/datastore/datastore/datastore_test.cpp
+++ b/vespalib/src/tests/datastore/datastore/datastore_test.cpp
@@ -17,7 +17,6 @@ using vespalib::alloc::MemoryAllocator;
 class MyStore : public DataStore<int, EntryRefT<3, 2> > {
 private:
     using ParentType = DataStore<int, EntryRefT<3, 2> >;
-    using ParentType::_primary_buffer_ids;
 public:
     MyStore() {}
     explicit MyStore(std::unique_ptr<BufferType<int>> type)
@@ -44,7 +43,7 @@ public:
     void switch_primary_buffer() {
         ParentType::switch_primary_buffer(0, 0u);
     }
-    size_t primary_buffer_id() const { return _primary_buffer_ids[0]; }
+    size_t primary_buffer_id() const { return get_primary_buffer_id(0); }
     BufferState& get_active_buffer_state() {
         return ParentType::getBufferState(primary_buffer_id());
     }
diff --git a/vespalib/src/vespa/vespalib/datastore/datastore.h b/vespalib/src/vespa/vespalib/datastore/datastore.h
index 75c77204837..6ff71238a89 100644
--- a/vespalib/src/vespa/vespalib/datastore/datastore.h
+++ b/vespalib/src/vespa/vespalib/datastore/datastore.h
@@ -86,7 +86,6 @@ class DataStore : public DataStoreT<RefT>
 protected:
     typedef DataStoreT<RefT> ParentType;
     using ParentType::ensureBufferCapacity;
-    using ParentType::_primary_buffer_ids;
     using ParentType::getEntry;
     using ParentType::dropBuffers;
     using ParentType::init_primary_buffers;
diff --git a/vespalib/src/vespa/vespalib/datastore/datastore.hpp b/vespalib/src/vespa/vespalib/datastore/datastore.hpp
index e14b248e569..18abe29d04f 100644
--- a/vespalib/src/vespa/vespalib/datastore/datastore.hpp
+++ b/vespalib/src/vespa/vespalib/datastore/datastore.hpp
@@ -44,19 +44,18 @@ template <typename RefT>
 void
 DataStoreT<RefT>::trimElemHoldList(generation_t usedGen)
 {
-    ElemHold2List &elemHold2List = _elemHold2List;
-
-    ElemHold2List::iterator it(elemHold2List.begin());
-    ElemHold2List::iterator ite(elemHold2List.end());
+    auto it = _elemHold2List.begin();
+    auto ite = _elemHold2List.end();
     uint32_t freed = 0;
     for (; it != ite; ++it) {
-        if (static_cast<sgeneration_t>(it->_generation - usedGen) >= 0)
+        if (static_cast<sgeneration_t>(it->_generation - usedGen) >= 0) {
             break;
+        }
         free_elem_internal(it->_ref, it->_len, true);
         ++freed;
     }
     if (freed != 0) {
-        elemHold2List.erase(elemHold2List.begin(), it);
+        _elemHold2List.erase(_elemHold2List.begin(), it);
     }
 }
 
@@ -64,14 +63,12 @@ template <typename RefT>
 void
 DataStoreT<RefT>::clearElemHoldList()
 {
-    ElemHold2List &elemHold2List = _elemHold2List;
-
-    ElemHold2List::iterator it(elemHold2List.begin());
-    ElemHold2List::iterator ite(elemHold2List.end());
+    auto it = _elemHold2List.begin();
+    auto ite = _elemHold2List.end();
     for (; it != ite; ++it) {
         free_elem_internal(it->_ref, it->_len, true);
     }
-    elemHold2List.clear();
+    _elemHold2List.clear();
 }
 
 template <typename RefT>
diff --git a/vespalib/src/vespa/vespalib/datastore/datastorebase.cpp b/vespalib/src/vespa/vespalib/datastore/datastorebase.cpp
index a5ed2c05f22..dd6c767e9c6 100644
--- a/vespalib/src/vespa/vespalib/datastore/datastorebase.cpp
+++ b/vespalib/src/vespa/vespalib/datastore/datastorebase.cpp
@@ -223,9 +223,8 @@ DataStoreBase::addType(BufferTypeBase *typeHandler)
 void
 DataStoreBase::transferElemHoldList(generation_t generation)
 {
-    ElemHold2List &elemHold2List = _elemHold2List;
-    for (const ElemHold1ListElem & elemHold1 : _elemHold1List) {
-        elemHold2List.push_back(ElemHold2ListElem(elemHold1, generation));
+    for (const auto& elemHold1 : _elemHold1List) {
+        _elemHold2List.push_back(ElemHold2ListElem(elemHold1, generation));
     }
     _elemHold1List.clear();
 }
@@ -289,14 +288,14 @@ DataStoreBase::holdBuffer(uint32_t bufferId)
 {
     _states[bufferId].onHold(bufferId);
     size_t holdBytes = 0u;  // getMemStats() still accounts held buffers
-    GenerationHeldBase::UP hold(new BufferHold(holdBytes, *this, bufferId));
+    auto hold = std::make_unique<BufferHold>(holdBytes, *this, bufferId);
     _genHolder.hold(std::move(hold));
 }
 
 void
 DataStoreBase::enableFreeLists()
 {
-    for (BufferState & bState : _states) {
+    for (auto& bState : _states) {
         if (!bState.isActive() || bState.getCompacting()) {
             continue;
         }
@@ -308,7 +307,7 @@ DataStoreBase::enableFreeLists()
 void
 DataStoreBase::disableFreeLists()
 {
-    for (BufferState & bState : _states) {
+    for (auto& bState : _states) {
         bState.disable_free_list();
     }
     _freeListsEnabled = false;
@@ -340,9 +339,9 @@ DataStoreBase::getMemStats() const
 {
     MemoryStats stats;
 
-    for (const BufferState & bState: _states) {
+    for (const auto& bState: _states) {
         auto typeHandler = bState.getTypeHandler();
-        BufferState::State state = bState.getState();
+        auto state = bState.getState();
         if ((state == BufferState::State::FREE) || (typeHandler == nullptr)) {
             ++stats._freeBuffers;
         } else if (state == BufferState::State::ACTIVE) {
@@ -370,7 +369,7 @@ DataStoreBase::getAddressSpaceUsage() const
     size_t usedArrays = 0;
     size_t deadArrays = 0;
     size_t limitArrays = 0;
-    for (const BufferState & bState: _states) {
+    for (const auto& bState: _states) {
         if (bState.isActive()) {
             uint32_t arraySize = bState.getArraySize();
             usedArrays += bState.size() / arraySize;
@@ -386,7 +385,7 @@ DataStoreBase::getAddressSpaceUsage() const
             LOG_ABORT("should not be reached");
         }
     }
-    return vespalib::AddressSpace(usedArrays, deadArrays, limitArrays);
+    return {usedArrays, deadArrays, limitArrays};
 }
 
 void
@@ -423,12 +422,11 @@ DataStoreBase::fallbackResize(uint32_t bufferId, size_t elemsNeeded)
     state.fallbackResize(bufferId, elemsNeeded,
                          _buffers[bufferId].get_atomic_buffer(),
                          toHoldBuffer);
-    GenerationHeldBase::UP
-        hold(new FallbackHold(oldAllocElems * elementSize,
-                              std::move(toHoldBuffer),
-                              oldUsedElems,
-                              state.getTypeHandler(),
-                              state.getTypeId()));
+    auto hold = std::make_unique<FallbackHold>(oldAllocElems * elementSize,
+                                               std::move(toHoldBuffer),
+                                               oldUsedElems,
+                                               state.getTypeHandler(),
+                                               state.getTypeId());
     if (!_initializing) {
         _genHolder.hold(std::move(hold));
     }
@@ -454,9 +452,15 @@ std::unique_ptr<CompactingBuffers>
 DataStoreBase::start_compact_worst_buffers(CompactionSpec compaction_spec, const CompactionStrategy& compaction_strategy)
 {
     // compact memory usage
-    CompactBufferCandidates elem_buffers(_numBuffers, compaction_strategy.get_max_buffers(), compaction_strategy.get_active_buffers_ratio(), compaction_strategy.getMaxDeadBytesRatio() / 2, CompactionStrategy::DEAD_BYTES_SLACK);
+    CompactBufferCandidates elem_buffers(_numBuffers, compaction_strategy.get_max_buffers(),
+                                         compaction_strategy.get_active_buffers_ratio(),
+                                         compaction_strategy.getMaxDeadBytesRatio() / 2,
+                                         CompactionStrategy::DEAD_BYTES_SLACK);
     // compact address space
-    CompactBufferCandidates array_buffers(_numBuffers, compaction_strategy.get_max_buffers(), compaction_strategy.get_active_buffers_ratio(), compaction_strategy.getMaxDeadAddressSpaceRatio() / 2, CompactionStrategy::DEAD_ADDRESS_SPACE_SLACK);
+    CompactBufferCandidates array_buffers(_numBuffers, compaction_strategy.get_max_buffers(),
+                                          compaction_strategy.get_active_buffers_ratio(),
+                                          compaction_strategy.getMaxDeadAddressSpaceRatio() / 2,
+                                          CompactionStrategy::DEAD_ADDRESS_SPACE_SLACK);
     uint32_t free_buffers = 0;
     for (uint32_t bufferId = 0; bufferId < _numBuffers; ++bufferId) {
         const auto &state = getBufferState(bufferId);
diff --git a/vespalib/src/vespa/vespalib/datastore/datastorebase.h b/vespalib/src/vespa/vespalib/datastore/datastorebase.h
index 712a171f51c..28c47207a9f 100644
--- a/vespalib/src/vespa/vespalib/datastore/datastorebase.h
+++ b/vespalib/src/vespa/vespalib/datastore/datastorebase.h
@@ -25,9 +25,10 @@ class CompactionStrategy;
  */
 class DataStoreBase
 {
-public:
+protected:
     /**
-     * Hold list before freeze, before knowing how long elements must be held.
+     * Hold list element used in the first phase of holding (before freeze),
+     * before knowing how long elements must be held.
      */
     class ElemHold1ListElem
     {
@@ -41,10 +42,27 @@ public:
         { }
     };
 
-protected:
     using generation_t = vespalib::GenerationHandler::generation_t;
     using sgeneration_t = vespalib::GenerationHandler::sgeneration_t;
 
+    /**
+     * Hold list element used in the second phase of holding (at freeze),
+     * when knowing how long elements must be held.
+     */
+    class ElemHold2ListElem : public ElemHold1ListElem
+    {
+    public:
+        generation_t _generation;
+
+        ElemHold2ListElem(const ElemHold1ListElem &hold1, generation_t generation)
+                : ElemHold1ListElem(hold1),
+                  _generation(generation)
+        { }
+    };
+
+    using ElemHold1List = vespalib::Array<ElemHold1ListElem>;
+    using ElemHold2List = std::deque<ElemHold2ListElem>;
+
 private:
     class BufferAndTypeId {
     public:
@@ -60,32 +78,16 @@ private:
         uint32_t   _typeId;
     };
     std::vector<BufferAndTypeId> _buffers; // For fast mapping with known types
-protected:
+
     // Provides a mapping from typeId -> primary buffer for that type.
     // The primary buffer is used for allocations of new element(s) if no available slots are found in free lists.
     std::vector<uint32_t> _primary_buffer_ids;
 
+protected:
     void* getBuffer(uint32_t bufferId) { return _buffers[bufferId].get_buffer_relaxed(); }
 
     /**
-     * Hold list at freeze, when knowing how long elements must be held
-     */
-    class ElemHold2ListElem : public ElemHold1ListElem
-    {
-    public:
-        generation_t _generation;
-
-        ElemHold2ListElem(const ElemHold1ListElem &hold1, generation_t generation)
-            : ElemHold1ListElem(hold1),
-              _generation(generation)
-        { }
-    };
-
-    using ElemHold1List = vespalib::Array<ElemHold1ListElem>;
-    using ElemHold2List = std::deque<ElemHold2ListElem>;
-
-    /**
-     * Class used to hold the old buffer as part of fallbackResize().
+     * Class used to hold the entire old buffer as part of fallbackResize().
      */
     class FallbackHold : public vespalib::GenerationHeldBase
     {
@@ -129,6 +131,7 @@ protected:
 
     virtual ~DataStoreBase();
 
+private:
     /**
      * Get the next buffer id after the given buffer id.
      */
@@ -138,6 +141,7 @@ protected:
             ret = 0;
         return ret;
     }
+protected:
 
     /**
      * Get the primary buffer for the given type id.
@@ -156,6 +160,7 @@ protected:
     virtual void clearElemHoldList() = 0;
 
     void markCompacting(uint32_t bufferId);
+
 public:
     uint32_t addType(BufferTypeBase *typeHandler);
     void init_primary_buffers();
@@ -191,9 +196,11 @@ public:
      */
     void switch_primary_buffer(uint32_t typeId, size_t elemsNeeded);
 
+private:
     bool consider_grow_active_buffer(uint32_t type_id, size_t elems_needed);
     void switch_or_grow_primary_buffer(uint32_t typeId, size_t elemsNeeded);
 
+public:
     vespalib::MemoryUsage getMemoryUsage() const;
 
     vespalib::AddressSpace getAddressSpaceUsage() const;
@@ -205,6 +212,8 @@ public:
     const BufferState &getBufferState(uint32_t bufferId) const { return _states[bufferId]; }
     BufferState &getBufferState(uint32_t bufferId) { return _states[bufferId]; }
     uint32_t getNumBuffers() const { return _numBuffers; }
+
+private:
     bool hasElemHold1() const { return !_elemHold1List.empty(); }
 
     /**
@@ -212,16 +221,19 @@ public:
      */
     void transferElemHoldList(generation_t generation);
 
+public:
     /**
      * Transfer holds from hold1 to hold2 lists, assigning generation.
      */
     void transferHoldLists(generation_t generation);
 
+private:
     /**
      * Hold of buffer has ended.
      */
     void doneHoldBuffer(uint32_t bufferId);
 
+public:
     /**
      * Trim hold lists, freeing buffers that no longer needs to be held.
      *
@@ -264,16 +276,14 @@ public:
      */
     void disableFreeLists();
 
+private:
     /**
      * Enable free list management.
      * This only works for fixed size elements.
      */
     void enableFreeList(uint32_t bufferId);
 
-    /**
-     * Disable free list management.
-     */
-    void disableFreeList(uint32_t bufferId);
+public:
     void disableElemHoldList();
 
     bool has_free_lists_enabled() const { return _freeListsEnabled; }
@@ -306,14 +316,18 @@ private:
     void onActive(uint32_t bufferId, uint32_t typeId, size_t elemsNeeded);
 
     void inc_hold_buffer_count();
+
 public:
     uint32_t getTypeId(uint32_t bufferId) const {
         return _buffers[bufferId].getTypeId();
     }
 
     void finishCompact(const std::vector<uint32_t> &toHold);
+
+private:
     void fallbackResize(uint32_t bufferId, size_t elementsNeeded);
 
+public:
     vespalib::GenerationHolder &getGenerationHolder() {
         return _genHolder;
     }