diff options
author | Håvard Pettersen <havardpe@yahooinc.com> | 2022-06-15 08:49:42 +0000 |
---|---|---|
committer | Håvard Pettersen <havardpe@yahooinc.com> | 2022-06-15 12:28:05 +0000 |
commit | 85c18b2ccf65bf4dccee5c72a50b9e13ed83fa55 (patch) | |
tree | b5f658a7b0f07b0c0bdea5afe408a18cea35c478 | |
parent | aaafe503173878c081aadaa91665cef162726a24 (diff) |
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 |