Stop bypassing hold list for btree nodes that were never reachable for readers.

7 files changed, 16 insertions, 45 deletions

diff --git a/vespalib/src/tests/datastore/datastore/datastore_test.cpp b/vespalib/src/tests/datastore/datastore/datastore_test.cpp
index 10b96a87444..645871d3ef6 100644
--- a/vespalib/src/tests/datastore/datastore/datastore_test.cpp
+++ b/vespalib/src/tests/datastore/datastore/datastore_test.cpp
@@ -670,7 +670,7 @@ TEST(DataStoreTest, control_static_sizes) {
 
 namespace {
 
-void test_free_element_to_held_buffer(bool direct, bool before_hold_buffer)
+void test_free_element_to_held_buffer(bool before_hold_buffer)
 {
     MyStore s;
     auto ref = s.addEntry(1);
@@ -679,19 +679,11 @@ void test_free_element_to_held_buffer(bool direct, bool before_hold_buffer)
     EXPECT_EQ(1u, s.primary_buffer_id());
     
     if (before_hold_buffer) {
-        if (direct) {
-            s.freeElem(ref, 1);
-        } else {
-            s.holdElem(ref, 1);
-        }
+        s.holdElem(ref, 1);
     }
     s.holdBuffer(0); // hold last buffer
     if (!before_hold_buffer) {
-        if (direct) {
-            ASSERT_DEATH({ s.freeElem(ref, 1); }, "isOnHold\\(\\) && was_held");
-        } else {
-            ASSERT_DEATH({ s.holdElem(ref, 1); }, "isActive\\(\\)");
-        }
+        ASSERT_DEATH({ s.holdElem(ref, 1); }, "isActive\\(\\)");
     }
     s.assign_generation(100);
     s.reclaim_memory(101);
@@ -699,25 +691,15 @@ void test_free_element_to_held_buffer(bool direct, bool before_hold_buffer)
 
 }
 
-TEST(DataStoreTest, free_to_active_then_held_buffer_is_ok)
-{
-    test_free_element_to_held_buffer(true, true);
-}
-
 TEST(DataStoreTest, hold_to_active_then_held_buffer_is_ok)
 {
-    test_free_element_to_held_buffer(false, true);
+    test_free_element_to_held_buffer(true);
 }
 
 #ifndef NDEBUG
-TEST(DataStoreDeathTest, free_to_held_buffer_is_not_ok)
-{
-    test_free_element_to_held_buffer(true, false);
-}
-
 TEST(DataStoreDeathTest, hold_to_held_buffer_is_not_ok)
 {
-    test_free_element_to_held_buffer(false, false);
+    test_free_element_to_held_buffer(false);
 }
 #endif
 
diff --git a/vespalib/src/vespa/vespalib/btree/btreenodeallocator.hpp b/vespalib/src/vespa/vespalib/btree/btreenodeallocator.hpp
index 4968bbaf4a7..a38b68afe73 100644
--- a/vespalib/src/vespa/vespalib/btree/btreenodeallocator.hpp
+++ b/vespalib/src/vespa/vespalib/btree/btreenodeallocator.hpp
@@ -235,7 +235,7 @@ freeze()
             InternalNodeType *inode = mapInternalRef(i);
             (void) inode;
             assert(inode->getFrozen());
-            _nodeStore.freeElem(i);
+            _nodeStore.holdElem(i);
         }
         _internalHoldUntilFreeze.clear();
     }
@@ -245,7 +245,7 @@ freeze()
             LeafNodeType *lnode = mapLeafRef(i);
             (void) lnode;
             assert(lnode->getFrozen());
-            _nodeStore.freeElem(i);
+            _nodeStore.holdElem(i);
         }
         _leafHoldUntilFreeze.clear();
     }
diff --git a/vespalib/src/vespa/vespalib/btree/btreenodestore.h b/vespalib/src/vespa/vespalib/btree/btreenodestore.h
index 7b89e2d0ddb..a63a4d20170 100644
--- a/vespalib/src/vespa/vespalib/btree/btreenodestore.h
+++ b/vespalib/src/vespa/vespalib/btree/btreenodestore.h
@@ -156,10 +156,6 @@ public:
         _store.holdElem(ref, 1);
     }
 
-    void freeElem(EntryRef ref) {
-        _store.freeElem(ref, 1);
-    }
-
     std::unique_ptr<vespalib::datastore::CompactingBuffers> start_compact_worst(const CompactionStrategy& compaction_strategy);
 
     void assign_generation(generation_t current_gen) {
diff --git a/vespalib/src/vespa/vespalib/datastore/bufferstate.cpp b/vespalib/src/vespa/vespalib/datastore/bufferstate.cpp
index e9ff243fb08..45a94693eeb 100644
--- a/vespalib/src/vespa/vespalib/datastore/bufferstate.cpp
+++ b/vespalib/src/vespa/vespalib/datastore/bufferstate.cpp
@@ -191,19 +191,17 @@ BufferState::hold_elems(size_t num_elems, size_t extra_bytes)
 }
 
 void
-BufferState::free_elems(EntryRef ref, size_t num_elems, bool was_held, size_t ref_offset)
+BufferState::free_elems(EntryRef ref, size_t num_elems, size_t ref_offset)
 {
     if (isActive()) {
         if (_free_list.enabled() && (num_elems == getArraySize())) {
             _free_list.push_entry(ref);
         }
     } else {
-        assert(isOnHold() && was_held);
+        assert(isOnHold());
     }
     _stats.inc_dead_elems(num_elems);
-    if (was_held) {
-        _stats.dec_hold_elems(num_elems);
-    }
+    _stats.dec_hold_elems(num_elems);
     getTypeHandler()->cleanHold(_buffer.get(), (ref_offset * _arraySize), num_elems,
                                 BufferTypeBase::CleanContext(_stats.extra_used_bytes_ref(),
                                                              _stats.extra_hold_bytes_ref()));
diff --git a/vespalib/src/vespa/vespalib/datastore/bufferstate.h b/vespalib/src/vespa/vespalib/datastore/bufferstate.h
index 0acad7d3ab1..3f023b41c51 100644
--- a/vespalib/src/vespa/vespalib/datastore/bufferstate.h
+++ b/vespalib/src/vespa/vespalib/datastore/bufferstate.h
@@ -100,7 +100,7 @@ public:
      * The given entry ref is put on the free list (if enabled).
      * Hold cleaning of elements is executed on the buffer type.
      */
-    void free_elems(EntryRef ref, size_t num_elems, bool was_held, size_t ref_offset);
+    void free_elems(EntryRef ref, size_t num_elems, size_t ref_offset);
 
     BufferStats& stats() { return _stats; }
     const BufferStats& stats() const { return _stats; }
diff --git a/vespalib/src/vespa/vespalib/datastore/datastore.h b/vespalib/src/vespa/vespalib/datastore/datastore.h
index 25b451deae7..95f47e98ef5 100644
--- a/vespalib/src/vespa/vespalib/datastore/datastore.h
+++ b/vespalib/src/vespa/vespalib/datastore/datastore.h
@@ -27,7 +27,7 @@ template <typename RefT = EntryRefT<22> >
 class DataStoreT : public DataStoreBase
 {
 private:
-    void free_elem_internal(EntryRef ref, size_t numElems, bool was_held);
+    void free_elem_internal(EntryRef ref, size_t numElems);
 
 public:
     typedef RefT RefType;
@@ -38,11 +38,6 @@ public:
     ~DataStoreT() override;
 
     /**
-     * Free element(s).
-     */
-    void freeElem(EntryRef ref, size_t numElems) { free_elem_internal(ref, numElems, false); }
-
-    /**
      * Hold element(s).
      */
     void holdElem(EntryRef ref, size_t numElems) {
diff --git a/vespalib/src/vespa/vespalib/datastore/datastore.hpp b/vespalib/src/vespa/vespalib/datastore/datastore.hpp
index 190ef994c0b..bfb63954875 100644
--- a/vespalib/src/vespa/vespalib/datastore/datastore.hpp
+++ b/vespalib/src/vespa/vespalib/datastore/datastore.hpp
@@ -22,11 +22,11 @@ DataStoreT<RefT>::~DataStoreT() = default;
 
 template <typename RefT>
 void
-DataStoreT<RefT>::free_elem_internal(EntryRef ref, size_t numElems, bool was_held)
+DataStoreT<RefT>::free_elem_internal(EntryRef ref, size_t numElems)
 {
     RefType intRef(ref);
     BufferState &state = getBufferState(intRef.bufferId());
-    state.free_elems(ref, numElems, was_held, intRef.offset());
+    state.free_elems(ref, numElems, intRef.offset());
 }
 
 template <typename RefT>
@@ -45,7 +45,7 @@ void
 DataStoreT<RefT>::reclaim_entry_refs(generation_t oldest_used_gen)
 {
     _entry_ref_hold_list.reclaim(oldest_used_gen, [this](const auto& elem) {
-        free_elem_internal(elem.ref, elem.num_elems, true);
+        free_elem_internal(elem.ref, elem.num_elems);
     });
 }
 
@@ -54,7 +54,7 @@ void
 DataStoreT<RefT>::reclaim_all_entry_refs()
 {
     _entry_ref_hold_list.reclaim_all([this](const auto& elem) {
-        free_elem_internal(elem.ref, elem.num_elems, true);
+        free_elem_internal(elem.ref, elem.num_elems);
     });
 }