Updated the approach after we introduced the use of canonlalize in DatabaseNodeImp.cpp

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

include/xrpl/basics/TaggedCache.h

@@ -17,6 +17,23 @@
 
 namespace xrpl {
 
+namespace detail {
+
+// Replace-policy tags used by TaggedCache::canonicalizeReplaceCache /
+// canonicalizeReplaceClient when calling canonicalize. With ReplaceCached the
+// cached value is always replaced with `data`, so `data` is never written back
+// and may be const. With ReplaceClient the cached value is kept and written
+// back into `data` (the client's pointer), which must therefore be writable.
+struct ReplaceCached
+{
+};
+
+struct ReplaceClient
+{
+};
+
+}  // namespace detail
+
 /** Map/cache combination.
     This class implements a cache and a map. The cache keeps objects alive
     in the map. The map allows multiple code paths that reference objects
@@ -96,6 +113,16 @@ public:
     bool
     del(key_type const& key, bool valid);
 
+private:
+    // Selects the `data` parameter type of canonicalize from the replace
+    // policy: const for detail::ReplaceCached (never written back), otherwise
+    // writable.
+    template <typename R>
+    using CanonicalizeClientPointerType = std::conditional_t<
+        std::is_same_v<detail::ReplaceCached, R>,
+        SharedPointerType const&,
+        SharedPointerType&>;
+
 public:
     /** Replace aliased objects with originals.
 
@@ -104,15 +131,23 @@ public:
         This routine eliminates the duplicate and performs a replacement
         on the callers shared pointer if needed.
 
+        `replacePolicy` is a callable taking the existing strong pointer and
+        returning whether to replace the cached value with `data` (true) or to
+        keep the cached value and write it back into `data` (false). Because the
+        write-back case mutates `data`, `data` must be writable.
+
+        (canonicalizeReplaceCache / canonicalizeReplaceClient call this with
+        internal replace-policy tags in place of a callable.)
+
         @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
+        @param replacePolicy A callable (existing strong pointer -> bool).
 
         @return `true` If the key already existed.
     */
     template <class R>
     bool
-    canonicalize(key_type const& key, SharedPointerType& data, R&& replaceCallback);
+    canonicalize(key_type const& key, CanonicalizeClientPointerType<R> data, R&& replacePolicy);
 
     bool
     canonicalizeReplaceCache(key_type const& key, SharedPointerType const& data);

include/xrpl/basics/TaggedCache.ipp

@@ -303,10 +303,26 @@ template <
 template <class R>
 inline bool
 TaggedCache<Key, T, IsKeyCache, SharedWeakUnionPointer, SharedPointerType, Hash, KeyEqual, Mutex>::
-    canonicalize(key_type const& key, SharedPointerType& data, R&& replaceCallback)
+    canonicalize(
+        key_type const& key,
+        CanonicalizeClientPointerType<R> data,
+        [[maybe_unused]] R&& replacePolicy)
 {
     // Return canonical value, store if needed, refresh in cache
     // Return values: true=we had the data already
+
+    // `replacePolicy` is one of:
+    //   - detail::ReplaceCached: always replace the cached value with `data`;
+    //     `data` is never written back and may be const.
+    //   - detail::ReplaceClient: keep the cached value and write it back into
+    //     `data` (the client's pointer), which must therefore be writable.
+    //   - a callable(strong pointer) -> bool: decide at run time; `data` must
+    //     be writable.
+    // For the latter two the write-back below requires a mutable `data`, so
+    // passing a const argument is a compile error.
+    using C = std::remove_cvref_t<R>;
+    constexpr bool replaceCached = std::is_same_v<C, detail::ReplaceCached>;
+
     std::scoped_lock const lock(mutex_);
 
     auto cit = cache_.find(key);
@@ -324,27 +340,28 @@ TaggedCache<Key, T, IsKeyCache, SharedWeakUnionPointer, SharedPointerType, Hash,
     Entry& entry = cit->second;
     entry.touch(clock_.now());
 
-    auto shouldReplace = [&] {
-        if constexpr (std::is_invocable_r_v<bool, R>)
+    auto shouldReplaceCached = [&] {
+        if constexpr (replaceCached)
         {
-            // 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();
+            return true;
+        }
+        else if constexpr (std::is_same_v<C, detail::ReplaceClient>)
+        {
+            return false;
         }
         else
         {
-            return replaceCallback(entry.ptr.getStrong());
+            return replacePolicy(entry.ptr.getStrong());
         }
     };
 
     if (entry.isCached())
     {
-        if (shouldReplace())
+        if (shouldReplaceCached())
         {
             entry.ptr = data;
         }
-        else
+        else if constexpr (!replaceCached)
         {
             data = entry.ptr.getStrong();
         }
@@ -356,11 +373,11 @@ TaggedCache<Key, T, IsKeyCache, SharedWeakUnionPointer, SharedPointerType, Hash,
 
     if (cachedData)
     {
-        if (shouldReplace())
+        if (shouldReplaceCached())
         {
             entry.ptr = data;
         }
-        else
+        else if constexpr (!replaceCached)
         {
             entry.ptr.convertToStrong();
             data = cachedData;
@@ -389,7 +406,7 @@ inline bool
 TaggedCache<Key, T, IsKeyCache, SharedWeakUnionPointer, SharedPointerType, Hash, KeyEqual, Mutex>::
     canonicalizeReplaceCache(key_type const& key, SharedPointerType const& data)
 {
-    return canonicalize(key, const_cast<SharedPointerType&>(data), []() { return true; });
+    return canonicalize(key, data, detail::ReplaceCached{});
 }
 
 template <
@@ -405,7 +422,7 @@ inline bool
 TaggedCache<Key, T, IsKeyCache, SharedWeakUnionPointer, SharedPointerType, Hash, KeyEqual, Mutex>::
     canonicalizeReplaceClient(key_type const& key, SharedPointerType& data)
 {
-    return canonicalize(key, data, []() { return false; });
+    return canonicalize(key, data, detail::ReplaceClient{});
 }
 
 template <

src/test/basics/TaggedCache_test.cpp

@@ -8,6 +8,7 @@
 #include <xrpl/protocol/Protocol.h>
 
 #include <memory>
+#include <utility>
 
 namespace xrpl {
 
@@ -133,6 +134,129 @@ 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.canonicalizeReplaceCache(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 == data;
+                }
+            };
+
+            using IntrPtrCache = TaggedCache<
+                Key,
+                MyRefCountObject,
+                /*IsKeyCache*/ false,
+                intr_ptr::SharedWeakUnionPtr<MyRefCountObject>,
+                intr_ptr::SharedPtr<MyRefCountObject>>;
+
+            IntrPtrCache intrPtrCache("IntrPtrTest", 1, 1s, clock, journal);
+
+            intrPtrCache.canonicalizeReplaceCache(1, intr_ptr::makeShared<MyRefCountObject>("one"));
+            BEAST_EXPECT(intrPtrCache.getCacheSize() == 1);
+            BEAST_EXPECT(intrPtrCache.size() == 1);
+
+            {
+                {
+                    intrPtrCache.canonicalizeReplaceCache(
+                        1, intr_ptr::makeShared<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.canonicalizeReplaceCache(
+                        1, intr_ptr::makeShared<MyRefCountObject>("one_replaced_2"));
+
+                    auto p2 = intrPtrCache.fetch(1);
+                    BEAST_EXPECT(*p2 == "one_replaced_2");
+
+                    intrPtrCache.del(1, true);
+                }
+
+                intrPtrCache.canonicalizeReplaceCache(
+                    1, intr_ptr::makeShared<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);
+        }
     }
 };