Take dead elements into account (and subtract them) when calculating how many arrays to allocate in a datastore buffer.

This avoids a problem were allocated memory can accumulate over time in components using an ArrayStore.
If all documents in an array attribute vector changes from one value class to another,
all elements in the buffers of the previous value class are marked dead.
Those buffers will eventually be compacted.
Without this fix the wanted size of the resulting compacted buffer is calculated too high,
and we allocate memory we are not going to use. If we move to yet another value class later,
the same problem occurs again and more memory is allocated.

1 files changed, 29 insertions, 4 deletions

diff --git a/searchlib/src/tests/attribute/compaction/attribute_compaction_test.cpp b/searchlib/src/tests/attribute/compaction/attribute_compaction_test.cpp
index 36a697eaa12..c3bb9481f0f 100644
--- a/searchlib/src/tests/attribute/compaction/attribute_compaction_test.cpp
+++ b/searchlib/src/tests/attribute/compaction/attribute_compaction_test.cpp
@@ -1,10 +1,11 @@
 // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-#include <vespa/vespalib/testkit/test_kit.h>
+#include <vespa/searchlib/attribute/address_space_usage.h>
 #include <vespa/searchlib/attribute/attribute.h>
 #include <vespa/searchlib/attribute/attributefactory.h>
 #include <vespa/searchlib/attribute/attributeguard.h>
 #include <vespa/searchlib/attribute/integerbase.h>
-#include <vespa/searchlib/attribute/address_space_usage.h>
+#include <vespa/vespalib/testkit/test_kit.h>
+#include <vespa/vespalib/util/stringfmt.h>
 
 #include <vespa/log/log.h>
 LOG_SETUP("attribute_compaction_test");
@@ -126,6 +127,12 @@ Config compactAddressSpaceAttributeConfig(bool enableAddressSpaceCompact)
 
 }
 
+double
+calc_alloc_waste(const AttributeStatus& status)
+{
+    return ((double)(status.getAllocated() - status.getUsed())) / status.getAllocated();
+}
+
 class Fixture {
 public:
     AttributePtr _v;
@@ -141,8 +148,9 @@ public:
     AttributeStatus getStatus() { _v->commit(true); return _v->getStatus(); }
     AttributeStatus getStatus(const vespalib::string &prefix) {
         AttributeStatus status(getStatus());
-        LOG(info, "status %s: used=%" PRIu64 ", dead=%" PRIu64 ", onHold=%" PRIu64,
-            prefix.c_str(), status.getUsed(), status.getDead(), status.getOnHold());
+        LOG(info, "status %s: allocated=%" PRIu64 ", used=%" PRIu64 ", dead=%" PRIu64 ", onHold=%" PRIu64 ", waste=%f",
+            prefix.c_str(), status.getAllocated(), status.getUsed(), status.getDead(), status.getOnHold(),
+            calc_alloc_waste(status));
         return status;
     }
     const Config &getConfig() const { return _v->getConfig(); }
@@ -167,6 +175,23 @@ TEST_F("Test that compaction of integer array attribute reduces memory usage", F
     EXPECT_LESS(afterStatus.getUsed(), beforeStatus.getUsed());
 }
 
+TEST_F("Allocated memory is not accumulated in an array attribute when moving between value classes when compaction is active",
+       Fixture({BasicType::INT64, CollectionType::ARRAY}))
+{
+    DocIdRange range = f.addDocs(1000);
+    for (uint32_t i = 0; i < 50; ++i) {
+        uint32_t values = 10 + i;
+        // When moving all documents from one value class to the next,
+        // all elements in the buffers of the previous value class are marked dead.
+        // Those buffers will eventually be compacted. By taking the dead elements into account when
+        // calculating how large the resulting compacted buffer should be,
+        // we don't accumulate allocated memory as part of that process.
+        f.populate(range, values);
+        auto status = f.getStatus(vespalib::make_string("values=%u", values));
+        EXPECT_LESS(calc_alloc_waste(status), 0.68);
+    }
+}
+
 void
 populate_and_hammer(Fixture& f, bool take_attribute_guard)
 {