Merge pull request #4681 from vespa-engine/geirst/minor-cleanup-in-searchlib-datastore

Geirst/minor cleanup in searchlib datastore

3 files changed, 66 insertions, 113 deletions

diff --git a/searchlib/src/tests/datastore/datastore/datastore_test.cpp b/searchlib/src/tests/datastore/datastore/datastore_test.cpp
index 7d990bc392e..7599f675b58 100644
--- a/searchlib/src/tests/datastore/datastore/datastore_test.cpp
+++ b/searchlib/src/tests/datastore/datastore/datastore_test.cpp
@@ -11,9 +11,14 @@ LOG_SETUP("datastore_test");
 namespace search {
 namespace datastore {
 
+struct IntReclaimer
+{
+    static void reclaim(int *) {}
+};
+
 class MyStore : public DataStore<int, EntryRefT<3, 2> > {
 private:
-    typedef DataStore<int, EntryRefT<3, 2> > ParentType;
+    using ParentType = DataStore<int, EntryRefT<3, 2> >;
     using ParentType::_activeBufferIds;
 public:
     MyStore() {}
@@ -125,30 +130,11 @@ public:
     MemoryUsage getMemoryUsage() const { return _store.getMemoryUsage(); }
 };
 
-typedef MyStore::RefType MyRef;
-
-class Test : public vespalib::TestApp {
-private:
-    bool assertMemStats(const DataStoreBase::MemStats & exp,
-                        const DataStoreBase::MemStats & act);
-    void requireThatEntryRefIsWorking();
-    void requireThatAlignedEntryRefIsWorking();
-    void requireThatEntriesCanBeAddedAndRetrieved();
-    void requireThatAddEntryTriggersChangeOfBuffer();
-    void requireThatWeCanHoldAndTrimBuffers();
-    void requireThatWeCanHoldAndTrimElements();
-    void requireThatWeCanUseFreeLists();
-    void requireThatMemoryStatsAreCalculated();
-    void requireThatMemoryUsageIsCalculated();
-    void requireThatWecanDisableElemHoldList();
-    void requireThatBufferGrowthWorks();
-public:
-    int Main() override;
-};
+using MyRef = MyStore::RefType;
 
 bool
-Test::assertMemStats(const DataStoreBase::MemStats & exp,
-                     const DataStoreBase::MemStats & act)
+assertMemStats(const DataStoreBase::MemStats &exp,
+               const DataStoreBase::MemStats &act)
 {
     if (!EXPECT_EQUAL(exp._allocElems, act._allocElems)) return false;
     if (!EXPECT_EQUAL(exp._usedElems, act._usedElems)) return false;
@@ -160,10 +146,9 @@ Test::assertMemStats(const DataStoreBase::MemStats & exp,
     return true;
 }
 
-void
-Test::requireThatEntryRefIsWorking()
+TEST("require that entry ref is working")
 {
-    typedef EntryRefT<22> MyRefType;
+    using MyRefType = EntryRefT<22>;
     EXPECT_EQUAL(4194304u, MyRefType::offsetSize());
     EXPECT_EQUAL(1024u, MyRefType::numBuffers());
     {
@@ -189,10 +174,9 @@ Test::requireThatEntryRefIsWorking()
     }
 }
 
-void
-Test::requireThatAlignedEntryRefIsWorking()
+TEST("require that aligned entry ref is working")
 {
-    typedef AlignedEntryRefT<22, 2> MyRefType; // 4 byte alignement
+    using MyRefType = AlignedEntryRefT<22, 2>; // 4 byte alignement
     EXPECT_EQUAL(4 * 4194304u, MyRefType::offsetSize());
     EXPECT_EQUAL(1024u, MyRefType::numBuffers());
     EXPECT_EQUAL(0u, MyRefType::align(0));
@@ -218,10 +202,9 @@ Test::requireThatAlignedEntryRefIsWorking()
     }
 }
 
-void
-Test::requireThatEntriesCanBeAddedAndRetrieved()
+TEST("require that entries can be added and retrieved")
 {
-    typedef DataStore<int> IntStore;
+    using IntStore = DataStore<int>;
     IntStore ds;
     EntryRef r1 = ds.addEntry(10);
     EntryRef r2 = ds.addEntry(20);
@@ -237,10 +220,9 @@ Test::requireThatEntriesCanBeAddedAndRetrieved()
     EXPECT_EQUAL(30, ds.getEntry(r3));
 }
 
-void
-Test::requireThatAddEntryTriggersChangeOfBuffer()
+TEST("require that add entry triggers change of buffer")
 {
-    typedef DataStore<uint64_t, EntryRefT<10, 10> > Store;
+    using Store = DataStore<uint64_t, EntryRefT<10, 10> >;
     Store s;
     uint64_t num = 0;
     uint32_t lastId = 0;
@@ -264,8 +246,7 @@ Test::requireThatAddEntryTriggersChangeOfBuffer()
     LOG(info, "Added %" PRIu64 " nums in 2 buffers", num);
 }
 
-void
-Test::requireThatWeCanHoldAndTrimBuffers()
+TEST("require that we can hold and trim buffers")
 {
     MyStore s;
     EXPECT_EQUAL(0u, MyRef(s.addEntry(1)).bufferId());
@@ -310,8 +291,7 @@ Test::requireThatWeCanHoldAndTrimBuffers()
     EXPECT_TRUE(s.getBufferState(3).size() == 0);
 }
 
-void
-Test::requireThatWeCanHoldAndTrimElements()
+TEST("require that we can hold and trim elements")
 {
     MyStore s;
     MyRef r1 = s.addEntry(1);
@@ -336,39 +316,44 @@ Test::requireThatWeCanHoldAndTrimElements()
     EXPECT_EQUAL(0, s.getEntry(r3));
 }
 
-void
-Test::requireThatWeCanUseFreeLists()
+MyRef
+toRef(Handle<int> handle)
+{
+    return MyRef(handle.ref);
+}
+
+TEST("require that we can use free lists")
 {
     MyStore s;
     s.enableFreeLists();
-    MyRef r1 = s.addEntry2(1);
-    s.holdElem(r1, 1);
+    auto allocator = s.freeListAllocator<IntReclaimer>();
+    auto h1 = allocator.alloc(1);
+    s.holdElem(h1.ref, 1);
     s.transferHoldLists(10);
-    MyRef r2 = s.addEntry2(2);
-    s.holdElem(r2, 1);
+    auto h2 = allocator.alloc(2);
+    s.holdElem(h2.ref, 1);
     s.transferHoldLists(20);
     s.trimElemHoldList(11);
-    MyRef r3 = s.addEntry2(3); // reuse r1
-    EXPECT_EQUAL(r1.offset(), r3.offset());
-    EXPECT_EQUAL(r1.bufferId(), r3.bufferId());
-    MyRef r4 = s.addEntry2(4);
-    EXPECT_EQUAL(r2.offset() + 1, r4.offset());
+    auto h3 = allocator.alloc(3); // reuse h1.ref
+    EXPECT_EQUAL(toRef(h1).offset(), toRef(h3).offset());
+    EXPECT_EQUAL(toRef(h1).bufferId(), toRef(h3).bufferId());
+    auto h4 = allocator.alloc(4);
+    EXPECT_EQUAL(toRef(h2).offset() + 1, toRef(h4).offset());
     s.trimElemHoldList(21);
-    MyRef r5 = s.addEntry2(5); // reuse r2
-    EXPECT_EQUAL(r2.offset(), r5.offset());
-    EXPECT_EQUAL(r2.bufferId(), r5.bufferId());
-    MyRef r6 = s.addEntry2(6);
-    EXPECT_EQUAL(r4.offset() + 1, r6.offset());
-    EXPECT_EQUAL(3, s.getEntry(r1));
-    EXPECT_EQUAL(5, s.getEntry(r2));
-    EXPECT_EQUAL(3, s.getEntry(r3));
-    EXPECT_EQUAL(4, s.getEntry(r4));
-    EXPECT_EQUAL(5, s.getEntry(r5));
-    EXPECT_EQUAL(6, s.getEntry(r6));
+    auto h5 = allocator.alloc(5); // reuse h2.ref
+    EXPECT_EQUAL(toRef(h2).offset(), toRef(h5).offset());
+    EXPECT_EQUAL(toRef(h2).bufferId(), toRef(h5).bufferId());
+    auto h6 = allocator.alloc(6);
+    EXPECT_EQUAL(toRef(h4).offset() + 1, toRef(h6).offset());
+    EXPECT_EQUAL(3, s.getEntry(h1.ref));
+    EXPECT_EQUAL(5, s.getEntry(h2.ref));
+    EXPECT_EQUAL(3, s.getEntry(h3.ref));
+    EXPECT_EQUAL(4, s.getEntry(h4.ref));
+    EXPECT_EQUAL(5, s.getEntry(h5.ref));
+    EXPECT_EQUAL(6, s.getEntry(h6.ref));
 }
 
-void
-Test::requireThatMemoryStatsAreCalculated()
+TEST("require that memory stats are calculated")
 {
     MyStore s;
     DataStoreBase::MemStats m;
@@ -421,8 +406,7 @@ Test::requireThatMemoryStatsAreCalculated()
     EXPECT_TRUE(assertMemStats(m, s.getMemStats()));
 }
 
-void
-Test::requireThatMemoryUsageIsCalculated()
+TEST("require that memory usage is calculated")
 {
     MyStore s;
     MyRef r = s.addEntry(10);
@@ -440,9 +424,7 @@ Test::requireThatMemoryUsageIsCalculated()
     s.trimHoldLists(101);
 }
 
-
-void
-Test::requireThatWecanDisableElemHoldList()
+TEST("require that we can disable elemement hold list")
 {
     MyStore s;
     MyRef r1 = s.addEntry(10);
@@ -471,8 +453,7 @@ Test::requireThatWecanDisableElemHoldList()
     s.trimHoldLists(101);
 }
 
-namespace
-{
+namespace {
 
 void assertGrowStats(GrowthStats expSizes,
                      GrowthStats expFirstBufSizes,
@@ -485,8 +466,8 @@ void assertGrowStats(GrowthStats expSizes,
 }
 
 }
-void
-Test::requireThatBufferGrowthWorks()
+
+TEST("require that buffer growth works")
 {
     // Always switch to new buffer, min size 4
     TEST_DO(assertGrowStats({ 4, 8, 16, 32, 64, 64, 64, 64, 64 },
@@ -494,7 +475,7 @@ Test::requireThatBufferGrowthWorks()
     // Resize if buffer size is less than 4, min size 0
     TEST_DO(assertGrowStats({ 4, 8, 16, 32, 64, 64, 64, 64, 64 },
                             { 0, 1, 2, 4 }, 4, 0, 4));
-    // Alwais switch to new buffer, min size 16
+    // Always switch to new buffer, min size 16
     TEST_DO(assertGrowStats({ 16, 32, 64, 64, 64, 64, 64, 64, 64 },
                             { 16 }, 68, 16, 0));
     // Resize if buffer size is less than 16, min size 0
@@ -508,28 +489,8 @@ Test::requireThatBufferGrowthWorks()
                             { 0, 1 }, 4, 0, 0));
 }
 
-int
-Test::Main()
-{
-    TEST_INIT("datastore_test");
-
-    requireThatEntryRefIsWorking();
-    requireThatAlignedEntryRefIsWorking();
-    requireThatEntriesCanBeAddedAndRetrieved();
-    requireThatAddEntryTriggersChangeOfBuffer();
-    requireThatWeCanHoldAndTrimBuffers();
-    requireThatWeCanHoldAndTrimElements();
-    requireThatWeCanUseFreeLists();
-    requireThatMemoryStatsAreCalculated();
-    requireThatMemoryUsageIsCalculated();
-    requireThatWecanDisableElemHoldList();
-    requireThatBufferGrowthWorks();
-
-    TEST_DONE();
-}
-
 }
 }
 
-TEST_APPHOOK(search::datastore::Test);
+TEST_MAIN() { TEST_RUN_ALL(); }
 
diff --git a/searchlib/src/vespa/searchlib/datastore/datastore.h b/searchlib/src/vespa/searchlib/datastore/datastore.h
index 926503e5816..accff67e2fa 100644
--- a/searchlib/src/vespa/searchlib/datastore/datastore.h
+++ b/searchlib/src/vespa/searchlib/datastore/datastore.h
@@ -84,7 +84,6 @@ class DataStore : public DataStoreT<RefT>
 protected:
     typedef DataStoreT<RefT> ParentType;
     using ParentType::ensureBufferCapacity;
-    // using ParentType::activeBuffer;
     using ParentType::_activeBufferIds;
     using ParentType::_freeListLists;
     using ParentType::getBufferEntry;
@@ -101,8 +100,10 @@ public:
     ~DataStore();
 
     EntryRef addEntry(const EntryType &e);
-    EntryRef addEntry2(const EntryType &e);
     const EntryType &getEntry(EntryRef ref) const;
+
+    template <typename ReclaimerT>
+    FreeListAllocator<EntryType, RefT, ReclaimerT> freeListAllocator();
 };
 
 extern template class DataStoreT<EntryRefT<22> >;
diff --git a/searchlib/src/vespa/searchlib/datastore/datastore.hpp b/searchlib/src/vespa/searchlib/datastore/datastore.hpp
index f01129803ce..74e3d9be2e0 100644
--- a/searchlib/src/vespa/searchlib/datastore/datastore.hpp
+++ b/searchlib/src/vespa/searchlib/datastore/datastore.hpp
@@ -163,23 +163,6 @@ DataStore<EntryType, RefT>::addEntry(const EntryType &e)
 }
 
 template <typename EntryType, typename RefT>
-EntryRef
-DataStore<EntryType, RefT>::addEntry2(const EntryType &e)
-{
-    BufferState::FreeListList &freeListList = _freeListLists[0];
-    if (freeListList._head == NULL)
-        return addEntry(e);
-    BufferState &state = *freeListList._head;
-    assert(state.isActive());
-    RefType ref(state.popFreeList());
-    EntryType *be =
-        this->template
-        getBufferEntry<EntryType>(ref.bufferId(), ref.offset());
-    *be = e;
-    return ref;
-}
-
-template <typename EntryType, typename RefT>
 const EntryType &
 DataStore<EntryType, RefT>::getEntry(EntryRef ref) const
 {
@@ -190,6 +173,14 @@ DataStore<EntryType, RefT>::getEntry(EntryRef ref) const
     return *be;
 }
 
+template <typename EntryType, typename RefT>
+template <typename ReclaimerT>
+FreeListAllocator<EntryType, RefT, ReclaimerT>
+DataStore<EntryType, RefT>::freeListAllocator()
+{
+    return FreeListAllocator<EntryType, RefT, ReclaimerT>(*this, 0);
+}
+
 extern template class DataStoreT<EntryRefT<22> >;
 
 }