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

[ximinez]

High Level Overview of Change

This PR, if merged, changes CachedView to only cache the key needed to find each ledger object (SLE) in the global CachedSLEs, instead of a strong reference to the SLE itself. Further, it releases a hard reference to the last Ledger used for path finding work as soon as there is no more work to be done. Additionally, a bunch of logging is added to monitor cache sizes before and after sweeping for expired objects. The logging is not strictly necessary for the solution, but it doesn't appear to hurt, and can be useful if there are similar issues in the future. Finally, several more objects, and some statistics for the CachedSLEs, are tracked for the get_counts admin command.

Resolves #4224

Context of Change

Currently, the CachedView object holds and keeps strong references (std::shared_ptr) to objects being loaded from the ledger. This prevents those objects from being completely released from the global cache (CachedSLEs, which is a TaggedCache) and releasing their memory until the owning Ledger is released.

This is not a significant problem during typical server operations, but can lead to memory exhaustion in some circumstances, including path finding. This looks like a memory leak, but isn't really, because the memory would be recovered eventually if rippled was able to continue.

It needs to be noted that this is not a guaranteed complete solution for all path finding memory issues. It's still possible that sufficiently large path_search configuration values, or a sufficiently large set of steps, accounts, and trust lines could still consume enough memory to cause problems.

Problems with memory usage during path finding has been an ongoing issue. See also:

• 417cfc2 (Proposed 1.8.4 #4061)
• Reduce memory requirements for RippleState #4080
• Incremental improvements to path finding memory usage #4099
• Further incremental improvements to path finding memory usage #4111
• Update documented pathfinding configuration defaults: #4409

I hope that this is the last word on this issue, but real world performance remains to be seen.

Type of Change

• [X ] Bug fix (non-breaking change which fixes an issue)
• [X ] Refactor (non-breaking change that only restructures code)

API Impact

• [X ] Public API: New feature (new methods and/or new fields)
  • Potentially adds several new key-value pairs to the output of get_counts.

Before / After

Before

Certain combinations of path finding requests could cause rippled to exhaust available memory and crash, or be killed by the operating system.

After

The same scenarios will not cause rippled to crash, or at worst will allow rippled to service many more requests and run a lot longer before crashing.

Test Plan

This is an optimization that shouldn't affect anything other than this particular edge case. Thus existing test cases should cover it.

Beta testers are encouraged to build this branch, put it on public facing servers with path finding configured, and report back on their results.

[intelliot]

• Requires additional testing since this makes changes that aren't necessarily specific to pathfinding.
• There's a smaller / more targeted change that could be made, which cleans out the pointers: just the changes in src/ripple/app/ledger/impl/LedgerMaster.cpp.
• Would like node operators (supporting pathfinding) to run this change to confirm that it resolves the problem.

[shortthefomo]

Feed back so far.

1. follow up responses to the initial path_find request seem slower than what we have today.. the production build seems to return results much more frequently.
2. memory seems to behave for the 2-3 hrs I let node run on "normal" requests and some light pathing requests.

3. asked few a few people to hit a front end that adds a bunch of load on the box and managed to bring it down. Here is error log...

2023-Dec-04 21:38:43.756954454 UTC NetworkOPs:WRN Missing offer
2023-Dec-04 21:38:43.757092579 UTC NetworkOPs:WRN Missing offer
2023-Dec-04 21:38:43.758713948 UTC NetworkOPs:WRN Missing offer
2023-Dec-04 21:38:43.759163464 UTC NetworkOPs:WRN Missing offer
2023-Dec-04 21:38:43.829825677 UTC Resource:WRN Consumer entry 127.0.0.1 dropped with balance 15010 at or above drop threshold 15000
2023-Dec-04 21:38:43.830112890 UTC Resource:WRN Consumer entry 127.0.0.1 dropped with balance 15104 at or above drop threshold 15000
rippled: /root/.conan/data/boost/1.82.0/_/_/package/3a52104b31a9c302344831090b42e066cefe22fe/include/boost/beast/websocket/detail/soft_mutex.hpp:89: bool boost::beast::websocket::detail::soft_mutex::try_lock(const T*) [with T = boost::beast::websocket::stream<boost::beast::basic_stream<boost::asio::ip::tcp, boost::asio::executor, boost::beast::unlimited_rate_policy> >::close_op<boost::asio::executor_binder<ripple::BaseWSPeer<ripple::ServerHandler, ripple::PlainWSPeer<ripple::ServerHandler> >::close(const boost::beast::websocket::close_reason&)::<lambda(const error_code&)>, boost::asio::strand<boost::asio::executor> > >]: Assertion `id_ != T::id' failed.
Aborted

[ximinez]

3. asked few a few people to hit a front end that adds a bunch of load on the box and managed to bring it down. Here is error log...

That looks like a problem at the websocket layer, which path finding shouldn't affect. Can you tell if the same issue occurs under heavy load without this change?

[shortthefomo]

Can you tell if the same issue occurs under heavy load without this change?

(Read the request wrong) *edited

I will build develop tomorrow and check latest less this, but I run that interface off the 1.12.0 prod build and it does not fail with this error under decent load.

[shortthefomo]

right did some more testing off the develop branch this morning @ximinez

can validate that the issue is present outside of this change as well.

[ximinez]

I run that interface off the 1.12.0 prod build and it does not fail with this error under decent load.

When you say "prod build", do you mean the published packages? Those have asserts disabled, so even if the issue exists there, you wouldn't see it. Could you build https://github.com/XRPLF/rippled/tree/1.12.0 the same way you built develop, and see if the issue exists there, too?

[intelliot]

right did some more testing off the develop branch this morning @ximinez

can validate that the issue is present outside of this change as well.

Given the issue appears on develop (without #4822 - and therefore doesn't seem to be related to this PR), can you open a new issue so we can track/discuss your findings there? @lathanbritz

[ximinez]

@Bronek @thejohnfreeman I pushed a commit to change the "miss" label to be lower-case. I don't think it requires re-review, but I wanted to give you a heads up.

[intelliot]

@sophiax851 this may require perf signoff

[mtrippled]

LGTM 👍

[intelliot]

Internal tracker: RPFC-79

[intelliot]

Perf SignedOff: Examined the tagged cache sweeping during the test during lab testing and using a Mainnet node, didn’t observe abnormal behavior. No performance regression observed either.

Released in 2.0.1.

[sophiax851]

It's worth mentioning that this "Perf SignedOff" testing was conducted to ensure that there is no performance regression in the normal transaction processing with the changes made in this PR which was intended to address the root cause of the issue that I identified for #4224 . There was separate testing conducted in verifying the #4224.