From e41e469e0264fe3fd184dd07e35a250bd3bcf6d9 Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Sun, 2 Aug 2026 06:40:54 -0400 Subject: [PATCH] perf: Move entries off `NodePathStack` instead of copying them Five sites copied the top entry out and then popped it. `SHAMapTreeNodePtr` is refcounted, so each copy bumped its atomic strong-ref count; `release()` moves the pointer out instead, a plain pointer swap with no atomic. `SHAMapNodeID` also derives from `CountedObject`, but `CountedObject` declares no move constructor, so moving a `SHAMapNodeID` still runs its copy constructor; `release()` saves nothing on the ID half of the pair. `dirtyUp` and `delItem` walk up to 64 levels per insert or delete on the ledger write path, so this removes up to 64 atomic operations per call, not 128. The two sites that read without popping now bind a reference rather than copying. `dirtyUp` and `delItem` both drop a `dynamicPointerCast`/null check for a static cast, on the reasoning that by the time either receives the stack, `addGiveItem`/`updateGiveItem` have already consumed the terminal leaf entry via `release()`, so every remaining entry is provably an inner node. That reasoning holds today, but an `XRPL_ASSERT` is a no-op under `NDEBUG`, so both get a real `UNREACHABLE`-guarded check instead: a release build that somehow violated the invariant would otherwise write through a misinterpreted node via `setChild`, silent memory corruption in place of the clean crash `dynamicPointerCast` used to produce. `updateGiveItem`'s own cast needed the same treatment for a different reason: an absent tag leaves an inner node on top of the stack, and the cast that followed the assertion there would have reinterpreted an `SHAMapInnerNode` as a `SHAMapLeafNode`. Replaced with `if (!top->isLeaf()) return false;`, pinned by a regression test. The `std::move` these sites previously applied to `staticPointerCast`/`dynamicPointerCast` was dropped rather than fixed: both only had a `TT const&` overload, so the move bound to that const ref and copied anyway, silently defeating the `SHAMapTreeNodePtr` refcount saving described above. Adds the missing rvalue overload to each, tied to `SharedIntrusive&&` rather than a bare `TT&&` so it cannot also bind to an lvalue in preference to the const-ref overload, and restores `std::move` at the three call sites that own a soon-to-be-discarded pointer. --- include/xrpl/basics/IntrusivePointer.h | 19 ++++++++++ include/xrpl/shamap/SHAMap.h | 26 ++++++++++++++ src/libxrpl/shamap/SHAMap.cpp | 49 +++++++++++++++++--------- src/tests/libxrpl/shamap/SHAMap.cpp | 15 ++++++++ 4 files changed, 92 insertions(+), 17 deletions(-) diff --git a/include/xrpl/basics/IntrusivePointer.h b/include/xrpl/basics/IntrusivePointer.h index b978016860..2f9f1084a6 100644 --- a/include/xrpl/basics/IntrusivePointer.h +++ b/include/xrpl/basics/IntrusivePointer.h @@ -529,11 +529,30 @@ staticPointerCast(TT const& v) return SharedPtr(StaticCastTagSharedIntrusive{}, v); } +// A bare `TT&&` here would be a forwarding reference, since TT is deduced directly from this +// parameter, and would then also bind to lvalues in preference to the `const&` overload above +// (binding to a plain reference beats binding to a const one), silently moving out of a caller's +// live variable on what looks like a copy call. Naming the wrapped type keeps TT nested inside +// SharedIntrusive, so this only binds to an actual rvalue of that type. +template +SharedPtr +staticPointerCast(SharedIntrusive&& v) +{ + return SharedPtr(StaticCastTagSharedIntrusive{}, std::move(v)); +} + template SharedPtr dynamicPointerCast(TT const& v) { return SharedPtr(DynamicCastTagSharedIntrusive{}, v); } + +template +SharedPtr +dynamicPointerCast(SharedIntrusive&& v) +{ + return SharedPtr(DynamicCastTagSharedIntrusive{}, std::move(v)); +} } // namespace intr_ptr } // namespace xrpl diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index 4652cfdba6..226626f363 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -490,6 +490,28 @@ private: stack_ = {}; } + /** + * Remove and return the node at the end of the path. + * + * Copying an entry out before popping costs an atomic increment on the node's refcount, + * and callers that pop immediately do this per level, so on a 64-level path it is + * measurable. Moving avoids that increment. + */ + [[nodiscard]] std::pair + release() + { + if (stack_.empty()) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::NodePathStack::release : empty stack"); + return {}; + // LCOV_EXCL_STOP + } + auto entry = std::move(stack_.top()); + stack_.pop(); + return entry; + } + /** * Start a path at the root of the map, whose ID is the zero-depth ID by definition. * @@ -601,6 +623,10 @@ private: * Walk towards the specified id, returning the node. Caller must check * if the return is nullptr, and if not, if the node->peekItem()->key() == * id + * + * @param stack records the path walked, or nullptr to skip recording it. Lookups that only + * want the leaf (see findKey) omit it to avoid building a path they would + * immediately discard. */ SHAMapLeafNode* walkTowardsKey(uint256 const& id, NodePathStack* stack = nullptr) const; diff --git a/src/libxrpl/shamap/SHAMap.cpp b/src/libxrpl/shamap/SHAMap.cpp index 03b6e9b6be..1118588f5b 100644 --- a/src/libxrpl/shamap/SHAMap.cpp +++ b/src/libxrpl/shamap/SHAMap.cpp @@ -111,10 +111,15 @@ SHAMap::dirtyUp(NodePathStack& stack, uint256 const& target, SHAMapTreeNodePtr c while (!stack.empty()) { - auto node = intr_ptr::dynamicPointerCast(stack.top().first); - SHAMapNodeID const nodeID = stack.top().second; - stack.pop(); - XRPL_ASSERT(node, "xrpl::SHAMap::dirtyUp : non-null node"); + auto [top, nodeID] = stack.release(); + if (!top->isInner()) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::dirtyUp : node is not inner"); + Throw(type_, target); + // LCOV_EXCL_STOP + } + auto node = intr_ptr::staticPointerCast(std::move(top)); auto const branch = selectBranch(nodeID, target); @@ -553,7 +558,7 @@ SHAMap::peekNextItem(uint256 const& id, NodePathStack& stack) const stack.pop(); while (!stack.empty()) { - auto const [node, nodeID] = stack.top(); + auto const& [node, nodeID] = stack.top(); XRPL_ASSERT(!node->isLeaf(), "xrpl::SHAMap::peekNextItem : another node is not leaf"); auto& inner = safeDowncast(*node); for (auto i = selectBranch(nodeID, id) + 1; i < kBranchFactor; ++i) @@ -611,7 +616,7 @@ SHAMap::boundHelper(uint256 const& id, BelowDirection direction) const walkTowardsKey(id, &stack); while (!stack.empty()) { - auto const [node, nodeID] = stack.top(); + auto const& [node, nodeID] = stack.top(); if (node->isLeaf()) { auto const& item = safeDowncast(*node).peekItem(); @@ -675,8 +680,7 @@ SHAMap::delItem(uint256 const& id) if (stack.empty()) Throw(type_, id); - auto leaf = intr_ptr::dynamicPointerCast(stack.top().first); - stack.pop(); + auto leaf = intr_ptr::dynamicPointerCast(stack.release().first); if (!leaf || (leaf->peekItem()->key() != id)) return false; @@ -688,9 +692,15 @@ SHAMap::delItem(uint256 const& id) while (!stack.empty()) { - auto node = intr_ptr::staticPointerCast(stack.top().first); - SHAMapNodeID const nodeID = stack.top().second; - stack.pop(); + auto [top, nodeID] = stack.release(); + if (!top->isInner()) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::delItem : node is not inner"); + Throw(type_, id); + // LCOV_EXCL_STOP + } + auto node = intr_ptr::staticPointerCast(std::move(top)); node = unshareNode(std::move(node), nodeID); node->setChild( @@ -761,8 +771,7 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr if (stack.empty()) Throw(type_, tag); - auto [node, nodeID] = stack.top(); - stack.pop(); + auto [node, nodeID] = stack.release(); if (node->isLeaf()) { @@ -849,11 +858,17 @@ SHAMap::updateGiveItem(SHAMapNodeType type, boost::intrusive_ptr(type_, tag); - auto node = intr_ptr::dynamicPointerCast(stack.top().first); - auto nodeID = stack.top().second; - stack.pop(); + auto [top, nodeID] = stack.release(); - if (!node || (node->peekItem()->key() != tag)) + // walkTowardsKey pushes an inner node's own entry before testing whether the branch it needs + // is empty, so a tag absent from the map leaves that inner node on top rather than a leaf. + // Not reachable through updateGiveItem's current callers, all of which check the item exists + // before calling this, but the static cast below is safe only once this is confirmed. + if (!top->isLeaf()) + return false; + auto node = intr_ptr::staticPointerCast(std::move(top)); + + if (node->peekItem()->key() != tag) { // LCOV_EXCL_START UNREACHABLE("xrpl::SHAMap::updateGiveItem : invalid node"); diff --git a/src/tests/libxrpl/shamap/SHAMap.cpp b/src/tests/libxrpl/shamap/SHAMap.cpp index 349f7c6fd9..8aef2fa464 100644 --- a/src/tests/libxrpl/shamap/SHAMap.cpp +++ b/src/tests/libxrpl/shamap/SHAMap.cpp @@ -417,6 +417,21 @@ TEST_F(SHAMapTraversal, bounds_agree_with_iteration_for_absent_keys) } } +TEST_F(SHAMapTraversal, update_give_item_on_absent_key_returns_false) +{ + tests::TestNodeFamily f{j_}; + auto keys = deepFanOutKeys(); + SHAMap map{SHAMapType::FREE, f}; + fillMap(map, keys); + + // Absent key: walkTowardsKey stops on an inner node with an empty branch, not a leaf. + auto const absentKey = uint256{std::string_view{std::string(64, '0')}}; + Buffer vuc{32}; + std::fill_n(vuc.data(), vuc.size(), std::uint8_t{2}); + EXPECT_FALSE(map.updateGiveItem( + SHAMapNodeType::TnAccountState, makeShamapitem(absentKey, std::move(vuc)))); +} + TEST_F(SHAMapTraversal, bounds_on_empty_map_return_end) { tests::TestNodeFamily f{j_};