Reduce number of lookups in LRU map for updates and invalidations

vespalib/src/tests/stllike/CMakeLists.txt

@@ -61,6 +61,7 @@ vespa_add_executable(vespalib_lrucache_test_app TEST
     lrucache.cpp
     DEPENDS
     vespalib
+    GTest::GTest
 )
 vespa_add_test(NAME vespalib_lrucache_test_app COMMAND vespalib_lrucache_test_app)
 vespa_add_executable(vespalib_cache_test_app TEST

vespalib/src/tests/stllike/lrucache.cpp

@@ -1,48 +1,48 @@
 // Copyright Vespa.ai. 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/vespalib/gtest/gtest.h>
 #include <vespa/vespalib/stllike/lrucache_map.hpp>
 #include <string>
 
 using namespace vespalib;
 
-TEST("testCache") {
-    lrucache_map< LruParam<int, std::string> > cache(7);
-    // Verfify start conditions.
-    EXPECT_TRUE(cache.size() == 0);
+TEST(LruCacheMapTest, cache_basics) {
+    lrucache_map<LruParam<int, std::string>> cache(7);
+    // Verify start conditions.
+    EXPECT_EQ(cache.size(), 0);
     cache.insert(1, "First inserted string");
     EXPECT_TRUE(cache.verifyInternals());
-    EXPECT_TRUE(cache.size() == 1);
+    EXPECT_EQ(cache.size(), 1);
     EXPECT_TRUE(cache.hasKey(1));
     cache.insert(2, "Second inserted string");
     EXPECT_TRUE(cache.verifyInternals());
-    EXPECT_TRUE(cache.size() == 2);
+    EXPECT_EQ(cache.size(), 2);
     EXPECT_TRUE(cache.hasKey(1));
     EXPECT_TRUE(cache.hasKey(2));
     cache.insert(3, "Third inserted string");
     EXPECT_TRUE(cache.verifyInternals());
-    EXPECT_TRUE(cache.size() == 3);
+    EXPECT_EQ(cache.size(), 3);
     EXPECT_TRUE(cache.hasKey(1));
     EXPECT_TRUE(cache.hasKey(2));
     EXPECT_TRUE(cache.hasKey(3));
     cache.insert(4, "Fourth inserted string");
     EXPECT_TRUE(cache.verifyInternals());
-    EXPECT_TRUE(cache.size() == 4);
+    EXPECT_EQ(cache.size(), 4);
     EXPECT_TRUE(cache.hasKey(1));
     EXPECT_TRUE(cache.hasKey(2));
     EXPECT_TRUE(cache.hasKey(3));
     EXPECT_TRUE(cache.hasKey(4));
     cache.insert(5, "Fifth inserted string");
     EXPECT_TRUE(cache.verifyInternals());
-    EXPECT_TRUE(cache.size() == 5);
+    EXPECT_EQ(cache.size(), 5);
     EXPECT_TRUE(cache.hasKey(1));
     EXPECT_TRUE(cache.hasKey(2));
     EXPECT_TRUE(cache.hasKey(3));
     EXPECT_TRUE(cache.hasKey(4));
     EXPECT_TRUE(cache.hasKey(5));
     cache.insert(6, "Sixt inserted string");
     EXPECT_TRUE(cache.verifyInternals());
-    EXPECT_TRUE(cache.size() == 6);
+    EXPECT_EQ(cache.size(), 6);
     EXPECT_TRUE(cache.hasKey(1));
     EXPECT_TRUE(cache.hasKey(2));
     EXPECT_TRUE(cache.hasKey(3));
@@ -51,7 +51,7 @@ TEST("testCache") {
     EXPECT_TRUE(cache.hasKey(6));
     cache.insert(7, "Seventh inserted string");
     EXPECT_TRUE(cache.verifyInternals());
-    EXPECT_EQUAL(cache.size(), 7u);
+    EXPECT_EQ(cache.size(), 7);
     EXPECT_TRUE(cache.hasKey(1));
     EXPECT_TRUE(cache.hasKey(2));
     EXPECT_TRUE(cache.hasKey(3));
@@ -61,7 +61,7 @@ TEST("testCache") {
     EXPECT_TRUE(cache.hasKey(7));
     cache.insert(8, "Eighth inserted string");
     EXPECT_TRUE(cache.verifyInternals());
-    EXPECT_EQUAL(cache.size(), 7u);
+    EXPECT_EQ(cache.size(), 7);
     EXPECT_TRUE(cache.hasKey(2));
     EXPECT_TRUE(cache.hasKey(3));
     EXPECT_TRUE(cache.hasKey(4));
@@ -71,7 +71,7 @@ TEST("testCache") {
     EXPECT_TRUE(cache.hasKey(8));
     cache.insert(15, "Eighth inserted string");
     EXPECT_TRUE(cache.verifyInternals());
-    EXPECT_EQUAL(cache.size(), 7u);
+    EXPECT_EQ(cache.size(), 7);
     EXPECT_TRUE(cache.hasKey(3));
     EXPECT_TRUE(cache.hasKey(4));
     EXPECT_TRUE(cache.hasKey(5));
@@ -80,7 +80,7 @@ TEST("testCache") {
     EXPECT_TRUE(cache.hasKey(8));
     EXPECT_TRUE(cache.hasKey(15));
     // Test get and erase
-    cache.get(3);
+    (void)cache.get(3);
     EXPECT_TRUE(cache.verifyInternals());
     cache.erase(3);
     EXPECT_TRUE(cache.verifyInternals());
@@ -91,135 +91,165 @@ using MyKey = std::shared_ptr<std::string>;
 using MyData = std::shared_ptr<std::string>;
 
 struct SharedEqual {
-    bool operator()(const MyKey & a, const MyKey & b) {
+    bool operator()(const MyKey& a, const MyKey& b) const noexcept {
         return ((*a) == (*b));
     }
 };
 
 struct SharedHash {
-    size_t operator() (const MyKey & arg) const { return arg->size(); }
+    size_t operator()(const MyKey& arg) const noexcept { return arg->size(); }
 };
 
 
-TEST("testCacheInsertOverResize") {
+TEST(LruCacheMapTest, cache_insert_over_resize) {
     using LS = std::shared_ptr<std::string>;
-    using Cache = lrucache_map< LruParam<int, LS> >;
+    using Cache = lrucache_map<LruParam<int, LS>>;
 
     Cache cache(100);
     size_t sum(0);
     for (size_t i(0); i < cache.capacity()*10; i++) {
-        LS s(new std::string("abc"));
+        LS s(std::make_shared<std::string>("abc"));
         cache[random()] = s;
         sum += strlen(s->c_str());
-        EXPECT_EQUAL(strlen(s->c_str()), s->size());
+        EXPECT_EQ(strlen(s->c_str()), s->size());
     }
-    EXPECT_EQUAL(sum, cache.capacity()*10*3);
+    EXPECT_EQ(sum, cache.capacity()*10*3);
 }
 
-TEST("testCacheErase") {
-    lrucache_map< LruParam<MyKey, MyData, SharedHash, SharedEqual> > cache(4);
+TEST(LruCacheMapTest, cache_erase_by_key) {
+    lrucache_map<LruParam<MyKey, MyData, SharedHash, SharedEqual>> cache(4);
 
-    MyData d(new std::string("foo"));
-    MyKey k(new std::string("barlol"));
-    // Verfify start conditions.
-    EXPECT_TRUE(cache.size() == 0);
-    EXPECT_TRUE(d.use_count() == 1);
-    EXPECT_TRUE(k.use_count() == 1);
+    MyData d(std::make_shared<std::string>("foo"));
+    MyKey k(std::make_shared<std::string>("barlol"));
+    // Verify start conditions.
+    EXPECT_EQ(cache.size(), 0);
+    EXPECT_EQ(d.use_count(), 1);
+    EXPECT_EQ(k.use_count(), 1);
     cache.insert(k, d);
-    EXPECT_TRUE(d.use_count() == 2);
-    EXPECT_TRUE(k.use_count() == 2);
+    EXPECT_EQ(d.use_count(), 2);
+    EXPECT_EQ(k.use_count(), 2);
     cache.erase(k);
-    EXPECT_TRUE(d.use_count() == 1);
-    EXPECT_TRUE(k.use_count() == 1);
+    EXPECT_EQ(d.use_count(), 1);
+    EXPECT_EQ(k.use_count(), 1);
 }
 
-TEST("testCacheIterator") {
-    using Cache = lrucache_map< LruParam<int, std::string> >;
+TEST(LruCacheMapTest, cache_iterator) {
+    using Cache = lrucache_map<LruParam<int, std::string>>;
     Cache cache(3);
     cache.insert(1, "first");
     cache.insert(2, "second");
     cache.insert(3, "third");
     Cache::iterator it(cache.begin());
     Cache::iterator mt(cache.end());
     ASSERT_TRUE(it != mt);
-    ASSERT_EQUAL("third", *it);
+    ASSERT_EQ("third", *it);
     ASSERT_TRUE(it != mt);
-    ASSERT_EQUAL("second", *(++it));
+    ASSERT_EQ("second", *(++it));
     ASSERT_TRUE(it != mt);
-    ASSERT_EQUAL("second", *it++);
+    ASSERT_EQ("second", *it++);
     ASSERT_TRUE(it != mt);
-    ASSERT_EQUAL("first", *it);
+    ASSERT_EQ("first", *it);
     ASSERT_TRUE(it != mt);
-    it++;
+    ++it;
     ASSERT_TRUE(it == mt);
     cache.insert(4, "fourth");
     Cache::iterator it2(cache.begin());
     Cache::iterator it3(cache.begin());
-    ASSERT_EQUAL("fourth", *it2);
+    ASSERT_EQ("fourth", *it2);
     ASSERT_TRUE(it2 == it3);
-    it2++;
+    ++it2;
     ASSERT_TRUE(it2 != it3);
-    it2++;
-    it2++;
+    ++it2;
+    ++it2;
     ASSERT_TRUE(it2 == mt);
     Cache::iterator it4 = cache.erase(it3);
-    ASSERT_EQUAL("third", *it4);
-    ASSERT_EQUAL("third", *cache.begin());
+    ASSERT_EQ("third", *it4);
+    ASSERT_EQ("third", *cache.begin());
     Cache::iterator it5(cache.erase(cache.end()));
     ASSERT_TRUE(it5 == cache.end());
 }
 
-TEST("testCacheIteratorErase") {
-    using Cache = lrucache_map< LruParam<int, std::string> >;
+namespace {
+
+template <typename C>
+std::string lru_key_order(C& cache) {
+    std::string keys;
+    for (auto it = cache.begin(); it != cache.end(); ++it) {
+        if (!keys.empty()) {
+            keys += ' ';
+        }
+        keys += std::to_string(it.key());
+    }
+    return keys;
+}
+
+}
+
+TEST(LruCacheMapTest, cache_erase_by_iterator) {
+    using Cache = lrucache_map<LruParam<int, std::string>>;
     Cache cache(3);
     cache.insert(1, "first");
     cache.insert(8, "second");
     cache.insert(15, "third");
     cache.insert(15, "third");
     cache.insert(8, "second");
     cache.insert(1, "first");
+    EXPECT_EQ(lru_key_order(cache), "1 8 15");
     Cache::iterator it(cache.begin());
-    ASSERT_EQUAL("first", *it);
-    it++;
-    ASSERT_EQUAL("second", *it);
+    ASSERT_EQ("first", *it);
+    ++it;
+    ASSERT_EQ("second", *it);
     it = cache.erase(it);
-    ASSERT_EQUAL("third", *it);
+    EXPECT_EQ(lru_key_order(cache), "1 15");
+    ASSERT_EQ("third", *it);
     cache.erase(it);
+    EXPECT_EQ(lru_key_order(cache), "1");
 }
 
-namespace {
+TEST(LruCacheMapTest, find_no_ref_returns_iterator_if_present_and_does_not_update_lru) {
+    using Cache = lrucache_map<LruParam<int, std::string>>;
+    Cache cache(3);
+    cache.insert(1, "ichi");
+    cache.insert(2, "ni");
+    cache.insert(3, "san");
+    EXPECT_EQ(lru_key_order(cache), "3 2 1");
 
-template <typename C>
-std::string lru_key_order(C& cache) {
-    std::string keys;
-    for (auto it = cache.begin(); it != cache.end(); ++it) {
-        keys += std::to_string(it.key());
-        keys += ' ';
-    }
-    return keys;
-}
+    auto iter = cache.find_no_ref(1);
+    ASSERT_TRUE(iter != cache.end());
+    EXPECT_EQ(*iter, "ichi");
+    EXPECT_EQ(lru_key_order(cache), "3 2 1");
+
+    iter = cache.find_no_ref(2);
+    ASSERT_TRUE(iter != cache.end());
+    EXPECT_EQ(*iter, "ni");
+    EXPECT_EQ(lru_key_order(cache), "3 2 1");
 
+    iter = cache.find_no_ref(4);
+    ASSERT_TRUE(iter == cache.end());
+    EXPECT_EQ(lru_key_order(cache), "3 2 1");
 }
 
-TEST("find_and_lazy_ref elides updating LRU head when less than half used") {
+TEST(LruCacheMapTest, find_and_lazy_ref_elides_updating_LRU_head_when_less_than_half_full) {
     using Cache = lrucache_map<LruParam<int, std::string>>;
     Cache cache(6);
     cache.insert(1, "a");
     cache.insert(2, "b");
-    EXPECT_EQUAL(lru_key_order(cache), "2 1 ");
-    EXPECT_NOT_EQUAL(cache.find_and_lazy_ref(1), nullptr);
-    EXPECT_EQUAL(lru_key_order(cache), "2 1 "); // Not updated
+    EXPECT_EQ(lru_key_order(cache), "2 1");
+    EXPECT_NE(cache.find_and_lazy_ref(1), nullptr);
+    EXPECT_EQ(lru_key_order(cache), "2 1"); // Not updated
     cache.insert(3, "c");
-    EXPECT_EQUAL(lru_key_order(cache), "3 2 1 ");
-    EXPECT_NOT_EQUAL(cache.find_and_lazy_ref(1), nullptr);
-    EXPECT_EQUAL(lru_key_order(cache), "3 2 1 "); // Still not > capacity/2
+    EXPECT_EQ(lru_key_order(cache), "3 2 1");
+    EXPECT_NE(cache.find_and_lazy_ref(1), nullptr);
+    EXPECT_EQ(lru_key_order(cache), "3 2 1"); // Still not > capacity/2
     cache.insert(4, "c");
-    EXPECT_EQUAL(lru_key_order(cache), "4 3 2 1 ");
-    EXPECT_NOT_EQUAL(cache.find_and_lazy_ref(1), nullptr);
-    EXPECT_EQUAL(lru_key_order(cache), "1 4 3 2 "); // At long last, our time to LRU shine
+    EXPECT_EQ(lru_key_order(cache), "4 3 2 1");
+    EXPECT_NE(cache.find_and_lazy_ref(1), nullptr);
+    EXPECT_EQ(lru_key_order(cache), "1 4 3 2"); // At long last, our time to LRU shine
+    EXPECT_EQ(cache.find_and_lazy_ref(5), nullptr); // Key not found
+    EXPECT_EQ(lru_key_order(cache), "1 4 3 2");
 }
 
-TEST("Eager find_and_ref always moves to LRU head") {
+TEST(LruCacheMapTest, eager_find_and_ref_always_moves_to_LRU_head) {
     using Cache = lrucache_map<LruParam<int, std::string>>;
     Cache cache(6);
     cache.insert(1, "a");
@@ -228,13 +258,15 @@ TEST("Eager find_and_ref always moves to LRU head") {
     cache.insert(4, "d");
     cache.insert(5, "e");
     cache.insert(6, "f");
-    EXPECT_EQUAL(lru_key_order(cache), "6 5 4 3 2 1 ");
-    EXPECT_NOT_EQUAL(cache.find_and_ref(2), nullptr);
-    EXPECT_EQUAL(lru_key_order(cache), "2 6 5 4 3 1 ");
-    EXPECT_NOT_EQUAL(cache.find_and_ref(5), nullptr);
-    EXPECT_EQUAL(lru_key_order(cache), "5 2 6 4 3 1 ");
-    EXPECT_NOT_EQUAL(cache.find_and_ref(1), nullptr);
-    EXPECT_EQUAL(lru_key_order(cache), "1 5 2 6 4 3 ");
+    EXPECT_EQ(lru_key_order(cache), "6 5 4 3 2 1");
+    EXPECT_NE(cache.find_and_ref(2), nullptr);
+    EXPECT_EQ(lru_key_order(cache), "2 6 5 4 3 1");
+    EXPECT_NE(cache.find_and_ref(5), nullptr);
+    EXPECT_EQ(lru_key_order(cache), "5 2 6 4 3 1");
+    EXPECT_NE(cache.find_and_ref(1), nullptr);
+    EXPECT_EQ(lru_key_order(cache), "1 5 2 6 4 3");
+    EXPECT_EQ(cache.find_and_ref(7), nullptr); // Key not found; no touching the shiny happy LRU
+    EXPECT_EQ(lru_key_order(cache), "1 5 2 6 4 3");
 }
 
-TEST_MAIN() { TEST_RUN_ALL(); }
+GTEST_MAIN_RUN_ALL_TESTS()

vespalib/src/vespa/vespalib/stllike/cache.h

@@ -106,12 +106,18 @@ class cache {
         [[nodiscard]] bool has_key(const KeyT& key) const noexcept;
         // Precondition: key does not already exist in the mapping
         void insert_and_update_size(const KeyT& key, ValueT value);
-        // Precondition: key must exist in the mapping
-        void erase_and_update_size(const KeyT& key);
-        void replace_and_update_size(const KeyT& key, ValueT value);
         // Fetches an existing key from the cache _without_ updating the LRU ordering.
         [[nodiscard]] const typename P::Value& get_existing(const KeyT& key) const;
 
+        // Returns true iff `key` existed in the mapping prior to the call, which also
+        // implies the mapping has been updated by consuming `value` (i.e. its contents
+        // has been std::move()'d away and it is now in a logically empty state).
+        // Otherwise, both the mapping and `value` remains unmodified and false is returned.
+        [[nodiscard]] bool try_replace_and_update_size(const KeyT& key, ValueT& value);
+        // Iff `key` was present in the mapping prior to the call, its entry is removed
+        // from the mapping and true is returned. Otherwise, the mapping remains unmodified
+        // and false is returned.
+        [[nodiscard]] bool try_erase_and_update_size(const KeyT& key);
         // Iff element exists in cache, assigns `val_out` the stored value and returns true.
         // This also updates the underlying LRU order.
         // Otherwise, `val_out` is not modified and false is returned.