Merge pull request #11293 from vespa-engine/geirst/enable-free-lists-for-multi-value-attributes

Enable free lists for multi-value mapping used in all multi-value att…

6 files changed, 94 insertions, 20 deletions

diff --git a/searchlib/src/tests/attribute/attribute_test.cpp b/searchlib/src/tests/attribute/attribute_test.cpp
index ce1344f5c26..f402f536ae2 100644
--- a/searchlib/src/tests/attribute/attribute_test.cpp
+++ b/searchlib/src/tests/attribute/attribute_test.cpp
@@ -2241,7 +2241,22 @@ void testNamePrefix() {
     EXPECT_EQUAL("sfsint32_pc", vS2->getNamePrefix());
     EXPECT_EQUAL("sfsint32_pc.xyz.abc", vSS1->getName());
     EXPECT_EQUAL("sfsint32_pc", vSS1->getNamePrefix());
+}
+
+class MyMultiValueAttribute : public ArrayStringAttribute {
+public:
+    MyMultiValueAttribute(const vespalib::string& name)
+        : ArrayStringAttribute(name, Config(BasicType::STRING, CollectionType::ARRAY))
+    {
+    }
+    bool has_free_lists_enabled() const { return this->_mvMapping.has_free_lists_enabled(); }
+};
 
+void
+test_multi_value_mapping_has_free_lists_enabled()
+{
+    MyMultiValueAttribute attr("mvtest");
+    EXPECT_TRUE(attr.has_free_lists_enabled());
 }
 
 void
@@ -2292,6 +2307,7 @@ int AttributeTest::Main()
     testReaderDuringLastUpdate();
     TEST_DO(testPendingCompaction());
     TEST_DO(testNamePrefix());
+    test_multi_value_mapping_has_free_lists_enabled();
 
     deleteDataDirs();
     TEST_DONE();
diff --git a/searchlib/src/tests/attribute/compaction/attribute_compaction_test.cpp b/searchlib/src/tests/attribute/compaction/attribute_compaction_test.cpp
index c3453783e38..2766d310baf 100644
--- a/searchlib/src/tests/attribute/compaction/attribute_compaction_test.cpp
+++ b/searchlib/src/tests/attribute/compaction/attribute_compaction_test.cpp
@@ -2,6 +2,7 @@
 #include <vespa/vespalib/testkit/test_kit.h>
 #include <vespa/searchlib/attribute/attribute.h>
 #include <vespa/searchlib/attribute/attributefactory.h>
+#include <vespa/searchlib/attribute/attributeguard.h>
 #include <vespa/searchlib/attribute/attributevector.hpp>
 #include <vespa/searchlib/attribute/integerbase.h>
 #include <vespa/searchlib/attribute/address_space_usage.h>
@@ -167,27 +168,63 @@ TEST_F("Test that compaction of integer array attribute reduces memory usage", F
     EXPECT_LESS(afterStatus.getUsed(), beforeStatus.getUsed());
 }
 
-TEST_F("Test that no compaction of int8 array attribute increases address space usage", Fixture(compactAddressSpaceAttributeConfig(false)))
+void
+populate_and_hammer(Fixture& f, bool take_attribute_guard)
 {
     DocIdRange range1 = f.addDocs(1000);
     DocIdRange range2 = f.addDocs(1000);
-    f.populate(range1, 1000);
-    f.hammer(range2, 101);
+    if (take_attribute_guard) {
+        {
+            // When attribute guard is held free lists will not be used in the hammer step.
+            search::AttributeGuard guard(f._v);
+            f.populate(range1, 1000);
+            f.hammer(range2, 101);
+        }
+        f._v->commit(true);
+        f._v->commit();
+    } else {
+        f.populate(range1, 1000);
+        f.hammer(range2, 101);
+    }
+}
+
+TEST_F("Address space usage (dead) increases significantly when free lists are NOT used (compaction configured off)",
+       Fixture(compactAddressSpaceAttributeConfig(false)))
+{
+    populate_and_hammer(f, true);
     AddressSpace afterSpace = f.getMultiValueAddressSpaceUsage("after");
     // 100 * 1000 dead arrays due to new values for docids
     // 1 reserved array accounted as dead
     EXPECT_EQUAL(100001u, afterSpace.dead());
 }
 
-TEST_F("Test that compaction of int8 array attribute limits address space usage", Fixture(compactAddressSpaceAttributeConfig(true)))
+TEST_F("Address space usage (dead) increases only slightly when free lists are used (compaction configured off)",
+       Fixture(compactAddressSpaceAttributeConfig(false)))
 {
-    DocIdRange range1 = f.addDocs(1000);
-    DocIdRange range2 = f.addDocs(1000);
-    f.populate(range1, 1000);
-    f.hammer(range2, 101);
+    populate_and_hammer(f, false);
+    AddressSpace afterSpace = f.getMultiValueAddressSpaceUsage("after");
+    // Only 1000 dead arrays (due to new values for docids) as free lists are used.
+    // 1 reserved array accounted as dead
+    EXPECT_EQUAL(1001u, afterSpace.dead());
+}
+
+TEST_F("Compaction limits address space usage (dead) when free lists are NOT used",
+       Fixture(compactAddressSpaceAttributeConfig(true)))
+{
+    populate_and_hammer(f, true);
     AddressSpace afterSpace = f.getMultiValueAddressSpaceUsage("after");
     // DEAD_ARRAYS_SLACK in multi value mapping is is 64k
     EXPECT_GREATER(65536u, afterSpace.dead());
 }
 
+TEST_F("Compaction is not executed when free lists are used",
+       Fixture(compactAddressSpaceAttributeConfig(true)))
+{
+    populate_and_hammer(f, false);
+    AddressSpace afterSpace = f.getMultiValueAddressSpaceUsage("after");
+    // Only 1000 dead arrays (due to new values for docids) as free lists are used.
+    // 1 reserved array accounted as dead
+    EXPECT_EQUAL(1001u, afterSpace.dead());
+}
+
 TEST_MAIN() { TEST_RUN_ALL(); }
diff --git a/searchlib/src/tests/attribute/multi_value_mapping/multi_value_mapping_test.cpp b/searchlib/src/tests/attribute/multi_value_mapping/multi_value_mapping_test.cpp
index 9bbcd71f59d..2f74e9d31f6 100644
--- a/searchlib/src/tests/attribute/multi_value_mapping/multi_value_mapping_test.cpp
+++ b/searchlib/src/tests/attribute/multi_value_mapping/multi_value_mapping_test.cpp
@@ -80,17 +80,19 @@ public:
           _maxSmallArraySize()
     {
     }
-    void setup(uint32_t maxSmallArraySize) {
-        _mvMapping = std::make_unique<MvMapping>(ArrayStoreConfig(maxSmallArraySize,
-                                                                  ArrayStoreConfig::AllocSpec(0, RefType::offsetSize(), 8 * 1024,
-                                                                                              ALLOC_GROW_FACTOR)));
+    void setup(uint32_t maxSmallArraySize, bool enable_free_lists = true) {
+        ArrayStoreConfig config(maxSmallArraySize,
+                                ArrayStoreConfig::AllocSpec(0, RefType::offsetSize(), 8 * 1024, ALLOC_GROW_FACTOR));
+        config.enable_free_lists(enable_free_lists);
+        _mvMapping = std::make_unique<MvMapping>(config);
         _attr = std::make_unique<AttributeType>(*_mvMapping);
         _maxSmallArraySize = maxSmallArraySize;
     }
-    void setup(uint32_t maxSmallArraySize, size_t minArrays, size_t maxArrays, size_t numArraysForNewBuffer) {
-        _mvMapping = std::make_unique<MvMapping>(ArrayStoreConfig(maxSmallArraySize,
-                                                                  ArrayStoreConfig::AllocSpec(minArrays, maxArrays, numArraysForNewBuffer,
-                                                                                              ALLOC_GROW_FACTOR)));
+    void setup(uint32_t maxSmallArraySize, size_t minArrays, size_t maxArrays, size_t numArraysForNewBuffer, bool enable_free_lists = true) {
+        ArrayStoreConfig config(maxSmallArraySize,
+                                ArrayStoreConfig::AllocSpec(minArrays, maxArrays, numArraysForNewBuffer, ALLOC_GROW_FACTOR));
+        config.enable_free_lists(enable_free_lists);
+        _mvMapping = std::make_unique<MvMapping>(config);
         _attr = std::make_unique<AttributeType>(*_mvMapping);
         _maxSmallArraySize = maxSmallArraySize;
     }
@@ -307,6 +309,18 @@ TEST_F(IntMappingTest, test_that_replace_works)
     EXPECT_EQ(4u, getTotalValueCnt());
 }
 
+TEST_F(IntMappingTest, test_that_free_lists_can_be_enabled)
+{
+    setup(3, true);
+    EXPECT_TRUE(_mvMapping->has_free_lists_enabled());
+}
+
+TEST_F(IntMappingTest, test_that_free_lists_can_be_disabled)
+{
+    setup(3, false);
+    EXPECT_FALSE(_mvMapping->has_free_lists_enabled());
+}
+
 TEST_F(CompactionIntMappingTest, test_that_compaction_works)
 {
     setup(3, 64, 512, 129);
diff --git a/searchlib/src/vespa/searchlib/attribute/multi_value_mapping.h b/searchlib/src/vespa/searchlib/attribute/multi_value_mapping.h
index 6246a656bf6..ca4303e772f 100644
--- a/searchlib/src/vespa/searchlib/attribute/multi_value_mapping.h
+++ b/searchlib/src/vespa/searchlib/attribute/multi_value_mapping.h
@@ -48,12 +48,14 @@ public:
 
     vespalib::AddressSpace getAddressSpaceUsage() const override;
     vespalib::MemoryUsage getArrayStoreMemoryUsage() const override;
+    bool has_free_lists_enabled() const { return _store.has_free_lists_enabled(); }
 
     static datastore::ArrayStoreConfig optimizedConfigForHugePage(size_t maxSmallArraySize,
                                                                   size_t hugePageSize,
                                                                   size_t smallPageSize,
                                                                   size_t minNumArraysForNewBuffer,
-                                                                  float allocGrowFactor);
+                                                                  float allocGrowFactor,
+                                                                  bool enable_free_lists);
 };
 
 }
diff --git a/searchlib/src/vespa/searchlib/attribute/multi_value_mapping.hpp b/searchlib/src/vespa/searchlib/attribute/multi_value_mapping.hpp
index 4460d1757a2..3dcc0df477d 100644
--- a/searchlib/src/vespa/searchlib/attribute/multi_value_mapping.hpp
+++ b/searchlib/src/vespa/searchlib/attribute/multi_value_mapping.hpp
@@ -80,9 +80,12 @@ MultiValueMapping<EntryT, RefT>::optimizedConfigForHugePage(size_t maxSmallArray
                                                              size_t hugePageSize,
                                                              size_t smallPageSize,
                                                              size_t minNumArraysForNewBuffer,
-                                                             float allocGrowFactor)
+                                                             float allocGrowFactor,
+                                                             bool enable_free_lists)
 {
-    return ArrayStore::optimizedConfigForHugePage(maxSmallArraySize, hugePageSize, smallPageSize, minNumArraysForNewBuffer, allocGrowFactor);
+    auto result = ArrayStore::optimizedConfigForHugePage(maxSmallArraySize, hugePageSize, smallPageSize, minNumArraysForNewBuffer, allocGrowFactor);
+    result.enable_free_lists(enable_free_lists);
+    return result;
 }
 
 }
diff --git a/searchlib/src/vespa/searchlib/attribute/multivalueattribute.hpp b/searchlib/src/vespa/searchlib/attribute/multivalueattribute.hpp
index f07efcafa9a..b23860ebd31 100644
--- a/searchlib/src/vespa/searchlib/attribute/multivalueattribute.hpp
+++ b/searchlib/src/vespa/searchlib/attribute/multivalueattribute.hpp
@@ -11,6 +11,7 @@ namespace search {
 namespace multivalueattribute {
 
 constexpr size_t SMALL_MEMORY_PAGE_SIZE = 4 * 1024;
+constexpr bool enable_free_lists = true;
 
 }
 
@@ -23,7 +24,8 @@ MultiValueAttribute(const vespalib::string &baseFileName,
                                                                vespalib::alloc::MemoryAllocator::HUGEPAGE_SIZE,
                                                                multivalueattribute::SMALL_MEMORY_PAGE_SIZE,
                                                                8 * 1024,
-                                                               cfg.getGrowStrategy().getMultiValueAllocGrowFactor()),
+                                                               cfg.getGrowStrategy().getMultiValueAllocGrowFactor(),
+                                                               multivalueattribute::enable_free_lists),
                  cfg.getGrowStrategy().to_generic_strategy())
 {
 }