support VESPA_SHARED_STRING_REPO_NO_RECLAIM flag

4 files changed, 78 insertions, 22 deletions

diff --git a/vespalib/src/tests/shared_string_repo/CMakeLists.txt b/vespalib/src/tests/shared_string_repo/CMakeLists.txt
index 9b788d9cbb7..6ae91b99411 100644
--- a/vespalib/src/tests/shared_string_repo/CMakeLists.txt
+++ b/vespalib/src/tests/shared_string_repo/CMakeLists.txt
@@ -5,4 +5,12 @@ vespa_add_executable(vespalib_shared_string_repo_test_app TEST
     DEPENDS
     vespalib
 )
-vespa_add_test(NAME vespalib_shared_string_repo_test_app COMMAND vespalib_shared_string_repo_test_app)
+vespa_add_test(
+  NAME vespalib_shared_string_repo_test_app
+  COMMAND vespalib_shared_string_repo_test_app
+)
+vespa_add_test(
+  NAME vespalib_shared_string_repo_test_app_no_reclaim
+  COMMAND vespalib_shared_string_repo_test_app
+  ENVIRONMENT "VESPA_SHARED_STRING_REPO_NO_RECLAIM=true"
+)
diff --git a/vespalib/src/tests/shared_string_repo/shared_string_repo_test.cpp b/vespalib/src/tests/shared_string_repo/shared_string_repo_test.cpp
index d128e5b8c49..acc710f8818 100644
--- a/vespalib/src/tests/shared_string_repo/shared_string_repo_test.cpp
+++ b/vespalib/src/tests/shared_string_repo/shared_string_repo_test.cpp
@@ -21,6 +21,14 @@ bool verbose = false;
 double budget = 0.10;
 size_t work_size = 4_Ki;
 
+size_t active_enums() {
+    return SharedStringRepo::stats().active_entries;
+}
+
+bool will_reclaim() {
+    return SharedStringRepo::will_reclaim();
+}
+
 //-----------------------------------------------------------------------------
 
 std::vector<vespalib::string> make_strings(size_t cnt) {
@@ -352,7 +360,7 @@ TEST("require that basic handle usage works") {
     Handle foo2("foo");
     Handle bar2("bar");
 
-    EXPECT_EQUAL(SharedStringRepo::stats().active_entries, 2u);
+    EXPECT_EQUAL(active_enums(), 2u);
 
     TEST_DO(verify_eq(empty, empty2));
     TEST_DO(verify_eq(foo, foo2));
@@ -375,31 +383,37 @@ TEST("require that basic handle usage works") {
 }
 
 TEST("require that handles can be copied") {
-    Handle a("foo");
+    size_t before = active_enums();
+    Handle a("copied");
+    EXPECT_EQUAL(active_enums(), before + 1);
     Handle b(a);
     Handle c;
     c = b;
-    EXPECT_EQUAL(SharedStringRepo::stats().active_entries, 1u);
+    EXPECT_EQUAL(active_enums(), before + 1);
     EXPECT_TRUE(a.id() == b.id());
     EXPECT_TRUE(b.id() == c.id());
-    EXPECT_EQUAL(c.as_string(), vespalib::string("foo"));
+    EXPECT_EQUAL(c.as_string(), vespalib::string("copied"));
 }
 
 TEST("require that handles can be moved") {
-    Handle a("foo");
+    size_t before = active_enums();
+    Handle a("moved");
+    EXPECT_EQUAL(active_enums(), before + 1);
     Handle b(std::move(a));
     Handle c;
     c = std::move(b);
-    EXPECT_EQUAL(SharedStringRepo::stats().active_entries, 1u);
+    EXPECT_EQUAL(active_enums(), before + 1);
     EXPECT_TRUE(a.id() == string_id());
     EXPECT_TRUE(b.id() == string_id());
-    EXPECT_EQUAL(c.as_string(), vespalib::string("foo"));
+    EXPECT_EQUAL(c.as_string(), vespalib::string("moved"));
 }
 
 TEST("require that handle/string can be obtained from string_id") {
+    size_t before = active_enums();
     Handle a("str");
+    EXPECT_EQUAL(active_enums(), before + 1);
     Handle b = Handle::handle_from_id(a.id());
-    EXPECT_EQUAL(SharedStringRepo::stats().active_entries, 1u);
+    EXPECT_EQUAL(active_enums(), before + 1);
     EXPECT_EQUAL(Handle::string_from_id(b.id()), vespalib::string("str"));
 }
 
@@ -419,19 +433,19 @@ TEST("require that handle can be self-assigned") {
 //-----------------------------------------------------------------------------
 
 void verify_direct(const vespalib::string &str, size_t value) {
-    size_t before = SharedStringRepo::stats().active_entries;
+    size_t before = active_enums();
     Handle handle(str);
     EXPECT_EQUAL(handle.id().hash(), value + 1);
     EXPECT_EQUAL(handle.id().value(), value + 1);
-    EXPECT_EQUAL(SharedStringRepo::stats().active_entries, before);
+    EXPECT_EQUAL(active_enums(), before);
     EXPECT_EQUAL(handle.as_string(), str);
 }
 
 void verify_not_direct(const vespalib::string &str) {
-    size_t before = SharedStringRepo::stats().active_entries;
+    size_t before = active_enums();
     Handle handle(str);
     EXPECT_EQUAL(handle.id().hash(), handle.id().value());
-    EXPECT_EQUAL(SharedStringRepo::stats().active_entries, before + 1);
+    EXPECT_EQUAL(active_enums(), before + 1);
     EXPECT_EQUAL(handle.as_string(), str);
 }
 
@@ -458,6 +472,7 @@ TEST("require that direct handles work as expected") {
 //-----------------------------------------------------------------------------
 
 TEST("require that basic multi-handle usage works") {
+    size_t before = active_enums();
     Handles a;
     a.reserve(4);
     Handle foo("foo");
@@ -467,7 +482,12 @@ TEST("require that basic multi-handle usage works") {
     a.push_back(foo.id());
     a.push_back(bar.id());
     Handles b(std::move(a));
-    EXPECT_EQUAL(SharedStringRepo::stats().active_entries, 2u);
+    if (will_reclaim()) {
+        EXPECT_EQUAL(before, 0u);
+        EXPECT_EQUAL(active_enums(), 2u);
+    } else {
+        EXPECT_EQUAL(active_enums(), before);
+    }
     EXPECT_EQUAL(a.view().size(), 0u);
     EXPECT_EQUAL(b.view().size(), 4u);
     EXPECT_TRUE(b.view()[0] == foo.id());
@@ -539,7 +559,15 @@ TEST("leak some handles on purpose") {
 #endif
 
 TEST("require that no handles have leaked during testing") {
-    EXPECT_EQUAL(SharedStringRepo::stats().active_entries, 0u);
+    if (will_reclaim()) {
+        EXPECT_EQUAL(active_enums(), 0u);
+    } else {
+        auto stats = SharedStringRepo::stats();
+        fprintf(stderr, "enum stats after testing (no reclaim):\n");
+        fprintf(stderr, "  active enums:   %zu\n", stats.active_entries);
+        fprintf(stderr, "  id space usage: %g\n", stats.id_space_usage());
+        fprintf(stderr, "  memory usage:   %zu\n", stats.memory_usage.usedBytes());
+    }
 }
 
 //-----------------------------------------------------------------------------
diff --git a/vespalib/src/vespa/vespalib/util/shared_string_repo.cpp b/vespalib/src/vespa/vespalib/util/shared_string_repo.cpp
index e46d5474eb9..5a09d617616 100644
--- a/vespalib/src/vespa/vespalib/util/shared_string_repo.cpp
+++ b/vespalib/src/vespa/vespalib/util/shared_string_repo.cpp
@@ -7,6 +7,17 @@ LOG_SETUP(".vespalib.shared_string_repo");
 
 namespace vespalib {
 
+namespace {
+
+bool resolve_should_reclaim_flag() {
+    bool no_reclaim = (getenv("VESPA_SHARED_STRING_REPO_NO_RECLAIM") != nullptr);
+    return !no_reclaim;
+}
+
+}
+
+const bool SharedStringRepo::should_reclaim = resolve_should_reclaim_flag();
+
 SharedStringRepo::Stats::Stats()
     : active_entries(0),
       total_entries(0),
@@ -86,8 +97,10 @@ SharedStringRepo SharedStringRepo::_repo;
 SharedStringRepo::SharedStringRepo() = default;
 SharedStringRepo::~SharedStringRepo()
 {
-    for (size_t p = 0; p < _partitions.size(); ++p) {
-        _partitions[p].find_leaked_entries(p);
+    if (should_reclaim) {
+        for (size_t p = 0; p < _partitions.size(); ++p) {
+            _partitions[p].find_leaked_entries(p);
+        }
     }
 }
 
@@ -116,8 +129,10 @@ SharedStringRepo::Handles::Handles(Handles &&rhs)
 
 SharedStringRepo::Handles::~Handles()
 {
-    for (string_id handle: _handles) {
-        _repo.reclaim(handle);
+    if (should_reclaim) {
+        for (string_id handle: _handles) {
+            _repo.reclaim(handle);
+        }
     }
 }
 
diff --git a/vespalib/src/vespa/vespalib/util/shared_string_repo.h b/vespalib/src/vespa/vespalib/util/shared_string_repo.h
index a55484537e2..260cac7428b 100644
--- a/vespalib/src/vespa/vespalib/util/shared_string_repo.h
+++ b/vespalib/src/vespa/vespalib/util/shared_string_repo.h
@@ -49,6 +49,7 @@ private:
     static constexpr uint32_t FAST_ID_MAX = 9999999;
     static constexpr uint32_t ID_BIAS = (FAST_ID_MAX + 2);
     static constexpr size_t PART_LIMIT = (std::numeric_limits<uint32_t>::max() - ID_BIAS) / NUM_PARTS;
+    static const bool should_reclaim;
 
     struct AltKey {
         vespalib::stringref str;
@@ -141,10 +142,13 @@ private:
         Stats stats() const;
 
         uint32_t resolve(const AltKey &alt_key) {
+            bool count_refs = should_reclaim;
             std::lock_guard guard(_lock);
             auto pos = _hash.find(alt_key);
             if (pos != _hash.end()) {
-                _entries[pos->idx].add_ref();
+                if (count_refs) {
+                    _entries[pos->idx].add_ref();
+                }
                 return pos->idx;
             } else {
                 uint32_t idx = make_entry(alt_key);
@@ -232,7 +236,7 @@ private:
     }
 
     string_id copy(string_id id) {
-        if (id._id >= ID_BIAS) {
+        if ((id._id >= ID_BIAS) && should_reclaim) {
             uint32_t part = (id._id - ID_BIAS) & PART_MASK;
             uint32_t local_idx = (id._id - ID_BIAS) >> PART_BITS;
             _partitions[part].copy(local_idx);
@@ -241,7 +245,7 @@ private:
     }
 
     void reclaim(string_id id) {
-        if (id._id >= ID_BIAS) {
+        if ((id._id >= ID_BIAS) && should_reclaim) {
             uint32_t part = (id._id - ID_BIAS) & PART_MASK;
             uint32_t local_idx = (id._id - ID_BIAS) >> PART_BITS;
             _partitions[part].reclaim(local_idx);
@@ -251,6 +255,7 @@ private:
     static SharedStringRepo _repo;
 
 public:
+    static bool will_reclaim() { return should_reclaim; }
     static Stats stats();
 
     // A single stand-alone string handle with ownership