Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve lifetime management of ledger objects (SLEs) to prevent runaway memory usage. AKA "Is it caching? It's always caching." #4822

Merged
merged 11 commits into from
Jan 12, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/ripple/app/ledger/impl/LedgerMaster.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1543,6 +1543,7 @@ LedgerMaster::updatePaths()
if (app_.getOPs().isNeedNetworkLedger())
{
--mPathFindThread;
mPathLedger.reset();
JLOG(m_journal.debug()) << "Need network ledger for updating paths";
return;
}
Expand All @@ -1568,6 +1569,7 @@ LedgerMaster::updatePaths()
else
{ // Nothing to do
--mPathFindThread;
mPathLedger.reset();
JLOG(m_journal.debug()) << "Nothing to do for updating paths";
return;
}
Expand All @@ -1584,6 +1586,7 @@ LedgerMaster::updatePaths()
<< "Published ledger too old for updating paths";
std::lock_guard ml(m_mutex);
--mPathFindThread;
mPathLedger.reset();
return;
}
}
Expand All @@ -1596,6 +1599,7 @@ LedgerMaster::updatePaths()
if (!pathRequests.requestsPending())
{
--mPathFindThread;
mPathLedger.reset();
JLOG(m_journal.debug())
<< "No path requests found. Nothing to do for updating "
"paths. "
Expand All @@ -1613,6 +1617,7 @@ LedgerMaster::updatePaths()
<< "No path requests left. No need for further updating "
"paths";
--mPathFindThread;
mPathLedger.reset();
return;
}
}
Expand Down
4 changes: 4 additions & 0 deletions src/ripple/app/paths/PathRequests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -200,13 +200,17 @@ PathRequests::updateAll(std::shared_ptr<ReadView const> const& inLedger)
break;
}

// Hold on to the line cache until after the lock is released, so it can
// be destroyed outside of the lock
std::shared_ptr<RippleLineCache> lastCache;
{
// Get the latest requests, cache, and ledger for next pass
std::lock_guard sl(mLock);

if (requests_.empty())
break;
requests = requests_;
lastCache = cache;
cache = getLineCache(cache->getLedger(), false);
}
} while (!app_.getJobQueue().isStopping());
Expand Down
5 changes: 1 addition & 4 deletions src/ripple/ledger/CachedView.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,7 @@ class CachedViewImpl : public DigestAwareReadView
DigestAwareReadView const& base_;
CachedSLEs& cache_;
std::mutex mutable mutex_;
std::unordered_map<
key_type,
std::shared_ptr<SLE const>,
hardened_hash<>> mutable map_;
std::unordered_map<key_type, uint256, hardened_hash<>> mutable map_;

public:
CachedViewImpl() = delete;
Expand Down
33 changes: 23 additions & 10 deletions src/ripple/ledger/impl/CachedView.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -33,25 +33,38 @@ CachedViewImpl::exists(Keylet const& k) const
std::shared_ptr<SLE const>
CachedViewImpl::read(Keylet const& k) const
{
{
static CountedObjects::Counter hits{"CachedView::cache_hit"};
static CountedObjects::Counter hitsexpired{"CachedView::cache_hit_expired"};
static CountedObjects::Counter misses{"CachedView::cache_miss"};
bool cacheHit = false;
bool baseRead = false;

auto const digest = [&]() -> std::optional<uint256> {
std::lock_guard lock(mutex_);
auto const iter = map_.find(k.key);
if (iter != map_.end())
{
if (!iter->second || !k.check(*iter->second))
return nullptr;
cacheHit = true;
return iter->second;
}
}
auto const digest = base_.digest(k.key);
return base_.digest(k.key);
}();
if (!digest)
return nullptr;
auto sle = cache_.fetch(*digest, [&]() { return base_.read(k); });
auto sle = cache_.fetch(*digest, [&]() {
baseRead = true;
return base_.read(k);
});
if (cacheHit && baseRead)
hitsexpired.increment();
else if (cacheHit)
hits.increment();
else
misses.increment();
std::lock_guard lock(mutex_);
auto const er = map_.emplace(k.key, sle);
auto const& iter = er.first;
auto const er = map_.emplace(k.key, *digest);
bool const inserted = er.second;
if (iter->second && !k.check(*iter->second))
if (sle && !k.check(*sle))
{
if (!inserted)
{
Expand All @@ -62,7 +75,7 @@ CachedViewImpl::read(Keylet const& k) const
}
return nullptr;
}
return iter->second;
return sle;
}

} // namespace detail
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/net/InfoSub.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ namespace ripple {
// Operations that clients may wish to perform against the network
// Master operational handler, server sequencer, network tracker

class InfoSubRequest
class InfoSubRequest : public CountedObject<InfoSubRequest>
{
public:
using pointer = std::shared_ptr<InfoSubRequest>;
Expand Down
4 changes: 3 additions & 1 deletion src/ripple/protocol/STAccount.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@
#ifndef RIPPLE_PROTOCOL_STACCOUNT_H_INCLUDED
#define RIPPLE_PROTOCOL_STACCOUNT_H_INCLUDED

#include <ripple/basics/CountedObject.h>
#include <ripple/protocol/AccountID.h>
#include <ripple/protocol/STBase.h>

#include <string>

namespace ripple {

class STAccount final : public STBase
class STAccount final : public STBase, public CountedObject<STAccount>
{
private:
// The original implementation of STAccount kept the value in an STBlob.
Expand Down
3 changes: 2 additions & 1 deletion src/ripple/protocol/STBitString.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#ifndef RIPPLE_PROTOCOL_STBITSTRING_H_INCLUDED
#define RIPPLE_PROTOCOL_STBITSTRING_H_INCLUDED

#include <ripple/basics/CountedObject.h>
#include <ripple/beast/utility/Zero.h>
#include <ripple/protocol/STBase.h>

Expand All @@ -30,7 +31,7 @@ namespace ripple {
// information of a template parameterized by an unsigned type. This RTTI
// information is needed to write gdb pretty printers.
template <int Bits>
class STBitString final : public STBase
class STBitString final : public STBase, public CountedObject<STBitString<Bits>>
{
static_assert(Bits > 0, "Number of bits must be positive");

Expand Down
6 changes: 4 additions & 2 deletions src/ripple/protocol/STBlob.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,18 @@
#define RIPPLE_PROTOCOL_STBLOB_H_INCLUDED

#include <ripple/basics/Buffer.h>
#include <ripple/basics/CountedObject.h>
#include <ripple/basics/Slice.h>
#include <ripple/protocol/STBase.h>

#include <cassert>
#include <cstring>
#include <memory>

namespace ripple {

// variable length byte string
class STBlob : public STBase
class STBlob : public STBase, public CountedObject<STBlob>
{
Buffer value_;

Expand Down Expand Up @@ -88,7 +90,7 @@ class STBlob : public STBase
};

inline STBlob::STBlob(STBlob const& rhs)
: STBase(rhs), value_(rhs.data(), rhs.size())
: STBase(rhs), CountedObject<STBlob>(rhs), value_(rhs.data(), rhs.size())
{
}

Expand Down
3 changes: 2 additions & 1 deletion src/ripple/protocol/STInteger.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,13 @@
#ifndef RIPPLE_PROTOCOL_STINTEGER_H_INCLUDED
#define RIPPLE_PROTOCOL_STINTEGER_H_INCLUDED

#include <ripple/basics/CountedObject.h>
#include <ripple/protocol/STBase.h>

namespace ripple {

template <typename Integer>
class STInteger : public STBase
class STInteger : public STBase, public CountedObject<STInteger<Integer>>
{
public:
using value_type = Integer;
Expand Down
2 changes: 1 addition & 1 deletion src/ripple/protocol/STIssue.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@

namespace ripple {

class STIssue final : public STBase
class STIssue final : public STBase, CountedObject<STIssue>
{
private:
Issue issue_{xrpIssue()};
Expand Down
3 changes: 2 additions & 1 deletion src/ripple/protocol/STVector256.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,14 @@
#ifndef RIPPLE_PROTOCOL_STVECTOR256_H_INCLUDED
#define RIPPLE_PROTOCOL_STVECTOR256_H_INCLUDED

#include <ripple/basics/CountedObject.h>
#include <ripple/protocol/STBase.h>
#include <ripple/protocol/STBitString.h>
#include <ripple/protocol/STInteger.h>

namespace ripple {

class STVector256 : public STBase
class STVector256 : public STBase, public CountedObject<STVector256>
{
std::vector<uint256> mValue;

Expand Down
3 changes: 2 additions & 1 deletion src/ripple/protocol/STXChainBridge.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#ifndef RIPPLE_PROTOCOL_STXCHAINBRIDGE_H_INCLUDED
#define RIPPLE_PROTOCOL_STXCHAINBRIDGE_H_INCLUDED

#include <ripple/basics/CountedObject.h>
#include <ripple/protocol/STAccount.h>
#include <ripple/protocol/STBase.h>
#include <ripple/protocol/STIssue.h>
Expand All @@ -29,7 +30,7 @@ namespace ripple {
class Serializer;
class STObject;

class STXChainBridge final : public STBase
class STXChainBridge final : public STBase, public CountedObject<STXChainBridge>
{
STAccount lockingChainDoor_{sfLockingChainDoor};
STIssue lockingChainIssue_{sfLockingChainIssue};
Expand Down
1 change: 0 additions & 1 deletion src/test/jtx/Env.h
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
#include <ripple/core/Config.h>
#include <ripple/json/json_value.h>
#include <ripple/json/to_string.h>
#include <ripple/ledger/CachedSLEs.h>
#include <ripple/protocol/Feature.h>
#include <ripple/protocol/Indexes.h>
#include <ripple/protocol/Issue.h>
Expand Down