From b7a066caa10c0a8e565a79cad3aa7dbabe9af722 Mon Sep 17 00:00:00 2001 From: JCW Date: Sat, 6 Jun 2026 18:13:47 +0100 Subject: [PATCH] Updated the approach after we introduced the use of canonlalize in DatabaseNodeImp.cpp --- include/xrpl/basics/TaggedCache.h | 50 ++++++--- include/xrpl/basics/TaggedCache.ipp | 163 ++++++++++------------------ 2 files changed, 96 insertions(+), 117 deletions(-) diff --git a/include/xrpl/basics/TaggedCache.h b/include/xrpl/basics/TaggedCache.h index e6883573c0..5941b6b7dd 100644 --- a/include/xrpl/basics/TaggedCache.h +++ b/include/xrpl/basics/TaggedCache.h @@ -13,11 +13,27 @@ #include #include #include -#include #include 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 @@ -97,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 + using CanonicalizeClientPointerType = std::conditional_t< + std::is_same_v, + SharedPointerType const&, + SharedPointerType&>; + public: /** Replace aliased objects with originals. @@ -105,21 +131,27 @@ 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 bool - canonicalize(key_type const& key, SharedPointerType& data, R&& replaceCallback); + canonicalize(key_type const& key, CanonicalizeClientPointerType data, R&& replacePolicy); - /** Replace the cache entry with the caller's data. */ bool canonicalizeReplaceCache(key_type const& key, SharedPointerType const& data); - /** Replace the caller's pointer with the cached data. */ bool canonicalizeReplaceClient(key_type const& key, SharedPointerType& data); @@ -261,14 +293,6 @@ private: using cache_type = hardened_partitioned_hash_map; - /** 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 - touchOrInsert(key_type const& key, SharedPointerType const& data); - [[nodiscard]] std::thread sweepHelper( clock_type::time_point const& whenExpire, diff --git a/include/xrpl/basics/TaggedCache.ipp b/include/xrpl/basics/TaggedCache.ipp index 3273fc1d43..dce18f9a14 100644 --- a/include/xrpl/basics/TaggedCache.ipp +++ b/include/xrpl/basics/TaggedCache.ipp @@ -291,50 +291,6 @@ TaggedCache -inline std::pair< - typename TaggedCache< - Key, - T, - IsKeyCache, - SharedWeakUnionPointer, - SharedPointerType, - Hash, - KeyEqual, - Mutex>::cache_type::Iterator, - bool> -TaggedCache:: - touchOrInsert(key_type const& key, SharedPointerType const& data) -{ - auto cit = cache_.find(key); - bool inserted = false; - - if (cit == cache_.end()) - { - inserted = true; - auto emplaceResult = cache_.emplace( - std::piecewise_construct, - std::forward_as_tuple(key), - std::forward_as_tuple(clock_.now(), data)); - - ++cacheCount_; - cit = emplaceResult.first; - return {cit, inserted}; - } - - cit->second.touch(clock_.now()); - - return {cit, inserted}; -} - template < class Key, class T, @@ -347,22 +303,65 @@ template < template inline bool TaggedCache:: - canonicalize(key_type const& key, SharedPointerType& data, R&& replaceCallback) + canonicalize( + key_type const& key, + CanonicalizeClientPointerType data, + [[maybe_unused]] R&& replacePolicy) { - std::lock_guard lock(mutex_); - auto [it, inserted] = touchOrInsert(key, data); - if (inserted) - return false; + // Return canonical value, store if needed, refresh in cache + // Return values: true=we had the data already - Entry& entry = it->second; + // `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; + constexpr bool replaceCached = std::is_same_v; + + std::scoped_lock const lock(mutex_); + + auto cit = cache_.find(key); + + if (cit == cache_.end()) + { + cache_.emplace( + std::piecewise_construct, + std::forward_as_tuple(key), + std::forward_as_tuple(clock_.now(), data)); + ++cacheCount_; + return false; + } + + Entry& entry = cit->second; + entry.touch(clock_.now()); + + auto shouldReplaceCached = [&] { + if constexpr (replaceCached) + { + return true; + } + else if constexpr (std::is_same_v) + { + return false; + } + else + { + return replacePolicy(entry.ptr.getStrong()); + } + }; if (entry.isCached()) { - if (replaceCallback(entry.ptr.getStrong())) + if (shouldReplaceCached()) { entry.ptr = data; } - else + else if constexpr (!replaceCached) { data = entry.ptr.getStrong(); } @@ -370,13 +369,15 @@ TaggedCache:: canonicalizeReplaceCache(key_type const& key, SharedPointerType const& data) { - std::lock_guard lock(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; - ++cacheCount_; - return true; - } - - entry.ptr = data; - ++cacheCount_; - return false; + return canonicalize(key, data, detail::ReplaceCached{}); } template < @@ -443,31 +422,7 @@ inline bool TaggedCache:: canonicalizeReplaceClient(key_type const& key, SharedPointerType& data) { - std::lock_guard lock(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; - ++cacheCount_; - return true; - } - - entry.ptr = data; - ++cacheCount_; - return false; + return canonicalize(key, data, detail::ReplaceClient{}); } template <