Address PR comments

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

include/xrpl/basics/TaggedCache.h

@@ -13,6 +13,7 @@
 #include <mutex>
 #include <thread>
 #include <type_traits>
+#include <utility>
 #include <vector>
 
 namespace xrpl {
@@ -97,26 +98,11 @@ public:
     del(key_type const& key, bool valid);
 
 public:
-    /** Replace aliased objects with originals.
-
-        Due to concurrency it is possible for two separate objects with
-        the same content and referring to the same unique "thing" to exist.
-        This routine eliminates the duplicate and performs a replacement
-        on the callers shared pointer if needed.
-
-        @param key The key corresponding to the object
-        @param data A shared pointer to the data corresponding to the object.
-        @param replace Function that decides if cache should be replaced
-
-        @return `true` If the key already existed.
-    */
-    template <class R>
-    bool
-    canonicalize(key_type const& key, SharedPointerType& data, R&& replaceCallback);
-
+    /** Replace the cache entry with the caller's data. */
     bool
     canonicalize_replace_cache(key_type const& key, SharedPointerType const& data);
 
+    /** Replace the caller's pointer with the cached data. */
     bool
     canonicalize_replace_client(key_type const& key, SharedPointerType& data);
 
@@ -260,6 +246,14 @@ private:
 
     using cache_type = hardened_partitioned_hash_map<key_type, Entry, Hash, KeyEqual>;
 
+    /** Look up key; if missing, emplace a new entry.
+        If found, touch the existing entry.
+        Caller must hold m_mutex.
+        @return {iterator, true} if newly inserted, {iterator, false} if found.
+    */
+    std::pair<typename cache_type::iterator, bool>
+    touchOrInsert(key_type const& key, SharedPointerType const& data);
+
     [[nodiscard]] std::thread
     sweepHelper(
         clock_type::time_point const& when_expire,

include/xrpl/basics/TaggedCache.ipp

@@ -304,80 +304,39 @@ template <
     class Hash,
     class KeyEqual,
     class Mutex>
-template <class R>
-inline bool
+inline std::pair<
+    typename TaggedCache<
+        Key,
+        T,
+        IsKeyCache,
+        SharedWeakUnionPointer,
+        SharedPointerType,
+        Hash,
+        KeyEqual,
+        Mutex>::cache_type::iterator,
+    bool>
 TaggedCache<Key, T, IsKeyCache, SharedWeakUnionPointer, SharedPointerType, Hash, KeyEqual, Mutex>::
-    canonicalize(key_type const& key, SharedPointerType& data, R&& replaceCallback)
+    touchOrInsert(key_type const& key, SharedPointerType const& data)
 {
-    // Return canonical value, store if needed, refresh in cache
-    // Return values: true=we had the data already
-    std::lock_guard lock(m_mutex);
-
     auto cit = m_cache.find(key);
+    bool inserted = false;
 
     if (cit == m_cache.end())
     {
-        m_cache.emplace(
+        inserted = true;
+        auto emplaceResult = m_cache.emplace(
             std::piecewise_construct,
             std::forward_as_tuple(key),
             std::forward_as_tuple(m_clock.now(), data));
-        ++m_cache_count;
-        return false;
-    }
-
-    Entry& entry = cit->second;
-    entry.touch(m_clock.now());
-
-    auto shouldReplace = [&] {
-        if constexpr (std::is_invocable_r_v<bool, R>)
-        {
-            // The reason for this extra complexity is for intrusive
-            // strong/weak combo getting a strong is relatively expensive
-            // and not needed for many cases.
-            return replaceCallback();
-        }
-        else
-        {
-            return replaceCallback(entry.ptr.getStrong());
-        }
-    };
-
-    if (entry.isCached())
-    {
-        if (shouldReplace())
-        {
-            entry.ptr = data;
-        }
-        else
-        {
-            data = entry.ptr.getStrong();
-        }
-
-        return true;
-    }
-
-    auto cachedData = entry.lock();
-
-    if (cachedData)
-    {
-        if (shouldReplace())
-        {
-            entry.ptr = data;
-        }
-        else
-        {
-            entry.ptr.convertToStrong();
-            data = cachedData;
-        }
 
         ++m_cache_count;
-        return true;
+        cit = emplaceResult.first;
+        return {cit, inserted};
     }
 
-    entry.ptr = data;
-    ++m_cache_count;
+    cit->second.touch(m_clock.now());
 
-    return false;
+    return {cit, inserted};
 }
 
 template <
@@ -393,7 +352,30 @@ inline bool
 TaggedCache<Key, T, IsKeyCache, SharedWeakUnionPointer, SharedPointerType, Hash, KeyEqual, Mutex>::
     canonicalize_replace_cache(key_type const& key, SharedPointerType const& data)
 {
-    return canonicalize(key, const_cast<SharedPointerType&>(data), []() { return true; });
+    std::lock_guard lock(m_mutex);
+
+    auto [it, inserted] = touchOrInsert(key, data);
+    if (inserted)
+        return false;
+
+    Entry& entry = it->second;
+
+    if (entry.isCached())
+    {
+        entry.ptr = data;
+        return true;
+    }
+
+    if (auto cachedData = entry.lock())
+    {
+        entry.ptr = data;
+        ++m_cache_count;
+        return true;
+    }
+
+    entry.ptr = data;
+    ++m_cache_count;
+    return false;
 }
 
 template <
@@ -409,7 +391,31 @@ inline bool
 TaggedCache<Key, T, IsKeyCache, SharedWeakUnionPointer, SharedPointerType, Hash, KeyEqual, Mutex>::
     canonicalize_replace_client(key_type const& key, SharedPointerType& data)
 {
-    return canonicalize(key, data, []() { return false; });
+    std::lock_guard lock(m_mutex);
+
+    auto [it, inserted] = touchOrInsert(key, data);
+    if (inserted)
+        return false;
+
+    Entry& entry = it->second;
+
+    if (entry.isCached())
+    {
+        data = entry.ptr.getStrong();
+        return true;
+    }
+
+    if (auto cachedData = entry.lock())
+    {
+        entry.ptr.convertToStrong();
+        data = cachedData;
+        ++m_cache_count;
+        return true;
+    }
+
+    entry.ptr = data;
+    ++m_cache_count;
+    return false;
 }
 
 template <

src/test/basics/TaggedCache_test.cpp

@@ -5,6 +5,8 @@
 #include <xrpl/basics/chrono.h>
 #include <xrpl/protocol/Protocol.h>
 
+#include <utility>
+
 namespace xrpl {
 
 /*
@@ -129,6 +131,130 @@ public:
             BEAST_EXPECT(c.getCacheSize() == 0);
             BEAST_EXPECT(c.getTrackSize() == 0);
         }
+        {
+            BEAST_EXPECT(!c.insert(5, "five"));
+            BEAST_EXPECT(c.getCacheSize() == 1);
+            BEAST_EXPECT(c.size() == 1);
+
+            {
+                auto const p1 = c.fetch(5);
+                BEAST_EXPECT(p1 != nullptr);
+                BEAST_EXPECT(c.getCacheSize() == 1);
+                BEAST_EXPECT(c.size() == 1);
+
+                // Advance the clock a lot
+                ++clock;
+                c.sweep();
+                BEAST_EXPECT(c.getCacheSize() == 0);
+                BEAST_EXPECT(c.size() == 1);
+
+                auto p2 = std::make_shared<std::string>("five_2");
+                BEAST_EXPECT(c.canonicalize_replace_cache(5, p2));
+                BEAST_EXPECT(c.getCacheSize() == 1);
+                BEAST_EXPECT(c.size() == 1);
+                // Make sure the caller's original pointer is unchanged
+                BEAST_EXPECT(p1.get() != p2.get());
+                BEAST_EXPECT(*p2 == "five_2");
+
+                auto const p3 = c.fetch(5);
+                BEAST_EXPECT(p3 != nullptr);
+                BEAST_EXPECT(p3.get() == p2.get());
+                BEAST_EXPECT(p3.get() != p1.get());
+            }
+
+            ++clock;
+            c.sweep();
+            BEAST_EXPECT(c.getCacheSize() == 0);
+            BEAST_EXPECT(c.size() == 0);
+        }
+
+        {
+            testcase("intrptr");
+
+            struct MyRefCountObject : IntrusiveRefCounts
+            {
+                std::string _data;
+
+                // Needed to support weak intrusive pointers
+                virtual void
+                partialDestructor() {};
+
+                MyRefCountObject() = default;
+                explicit MyRefCountObject(std::string data) : _data(std::move(data))
+                {
+                }
+                explicit MyRefCountObject(char const* data) : _data(data)
+                {
+                }
+
+                MyRefCountObject&
+                operator=(MyRefCountObject const& other)
+                {
+                    _data = other._data;
+                    return *this;
+                }
+
+                bool
+                operator==(MyRefCountObject const& other) const
+                {
+                    return _data == other._data;
+                }
+
+                bool
+                operator==(std::string const& other) const
+                {
+                    return _data == other;
+                }
+            };
+
+            using IntrPtrCache = TaggedCache<
+                Key,
+                MyRefCountObject,
+                /*IsKeyCache*/ false,
+                intr_ptr::SharedWeakUnionPtr<MyRefCountObject>,
+                intr_ptr::SharedPtr<MyRefCountObject>>;
+
+            IntrPtrCache intrPtrCache("IntrPtrTest", 1, 1s, clock, journal);
+
+            intrPtrCache.canonicalize_replace_cache(
+                1, intr_ptr::make_shared<MyRefCountObject>("one"));
+            BEAST_EXPECT(intrPtrCache.getCacheSize() == 1);
+            BEAST_EXPECT(intrPtrCache.size() == 1);
+
+            {
+                {
+                    intrPtrCache.canonicalize_replace_cache(
+                        1, intr_ptr::make_shared<MyRefCountObject>("one_replaced"));
+
+                    auto p = intrPtrCache.fetch(1);
+                    BEAST_EXPECT(*p == "one_replaced");
+
+                    // Advance the clock a lot
+                    ++clock;
+                    intrPtrCache.sweep();
+                    BEAST_EXPECT(intrPtrCache.getCacheSize() == 0);
+                    BEAST_EXPECT(intrPtrCache.size() == 1);
+
+                    intrPtrCache.canonicalize_replace_cache(
+                        1, intr_ptr::make_shared<MyRefCountObject>("one_replaced_2"));
+
+                    auto p2 = intrPtrCache.fetch(1);
+                    BEAST_EXPECT(*p2 == "one_replaced_2");
+
+                    intrPtrCache.del(1, true);
+                }
+
+                intrPtrCache.canonicalize_replace_cache(
+                    1, intr_ptr::make_shared<MyRefCountObject>("one_replaced_3"));
+                auto p3 = intrPtrCache.fetch(1);
+                BEAST_EXPECT(*p3 == "one_replaced_3");
+            }
+
+            ++clock;
+            intrPtrCache.sweep();
+            BEAST_EXPECT(intrPtrCache.getCacheSize() == 0);
+            BEAST_EXPECT(intrPtrCache.size() == 0);
+        }
     }
 };