Incremental improvements to path finding memory usage #4099

ximinez · 2022-02-12T01:14:15Z

High Level Overview of Change

As the number of trust lines on the ledger has exploded, so has the amount of memory used by servers for path finding.

Some particularly gnarly requests have been observed to load over 20 million trust lines into memory. These changes build on #4080, which reduces the amount of memory used per trust line, so reviewers only need to review the commit(s) starting at "Incremental improvements to path finding memory usage".

Background path finding jobs can continue after a websocket client has created another request, or disconnected entirely. This can cause a lot of memory to be used unnecessarily while other requests are using even more memory. The main changes in this PR free up the relevant memory more quickly when the request is no longer relevant.

Type of Change

[X ] New feature (non-breaking change which adds functionality)
[X ] Refactor (non-breaking change that only restructures code)

Test Plan

Run an expensive path_find request. Wait until it returns a full_reply: true result, indicating that the background job has started. Invalidate the request using one of the following methods:

Disconnect the client.
Issue a { command: path_find, subcommand: close } to close the request.
Issue a new path_find request on the same connection.

With version of code before this PR, the path request can be observed to continue running by tracking process memory, or using the get_counts command to see the number of PathFindTrustLine continue to grow. (Note that the counter for PathFindTrustLine was only added in #4080.) With these changes, the memory usage will drop within about a few seconds to a minute, and the PathFindTrustLine count will drop to 0 for all cases but the last.

Future Tasks

Now that the trust line memory is getting freed up more quickly, I'm working on preventing as many trust lines and other objects from being loaded in the first place.

seelabs

Looks good. Left some minor comments, but nothing big.

seelabs · 2022-02-15T20:01:33Z

src/ripple/app/paths/PathRequests.cpp

+            << "getLineCache creating new cache for " << lgrSeq;
+        // Assign to the local before the member, because the member is a
+        // weak_ptr, and will immediately discard it if there are no other
+        // references.


Nice comment. Thanks for that.

seelabs · 2022-02-15T20:02:00Z

src/ripple/app/paths/PathRequests.cpp

@@ -80,15 +90,28 @@ PathRequests::updateAll(

    int processed = 0, removed = 0;

+    auto getSubscriber = [](PathRequest::pointer& request) -> InfoSub::pointer {


Param could be const&

seelabs · 2022-02-15T20:05:50Z

src/ripple/app/paths/PathRequests.cpp

+                            Json::Value update = request->doUpdate(
+                                cache, false, [&getSubscriber, &request]() {
+                                    return (bool)getSubscriber(request);
+                                });


It's not obvious what that lambda is doing. Maybe make a local variable for the callback just for the purpose of nameing it? I.e.

auto conintueCallback = [&getSubscriber, &request]()...

seelabs · 2022-02-15T20:08:36Z

src/ripple/app/paths/Pathfinder.cpp

+            s << ", " << *iter;
+    }
+    return s;
+}


This kind of iterator comes up all the time - I wish someone would add it to boost or std. This is the best I could find:
https://en.cppreference.com/w/cpp/experimental/ostream_joiner
but that's experimental, I'd rather not use that.

I couldn't help myself, so I wrote something to do it.

seelabs · 2022-02-15T20:14:20Z

src/ripple/app/paths/PathRequest.h

+    doUpdate(
+        std::shared_ptr<RippleLineCache> const&,
+        bool fast,
+        std::function<bool(void)> continueCallback = {});


Consider passing function by const& (here and other places). Here's a patch to do that if you want: seelabs@97ede1e

mtrippled

👍 LGTM

seelabs

I think this looks good. And thanks for coding up the join utility. That's awesome!

I did have a concern about lifetime management of the delimiter. Take a look and see what you think.

seelabs · 2022-02-25T20:19:01Z

src/ripple/basics/join.h

+{
+public:
+    Collection const& collection;
+    Str const& delimiter;


The lifetime management of delimiter gives me pause. In the common case - passing a statically allocated c string - we're fine. But this is a very general class and I could see delimiter pointing to some temporary that went out of scope. Since delimiters are going to be small (usually one or two characters) and std::string has a small-object optimization (no allocations for small strings), what do you think about using a std::string for the delimiter instead?

FWIW, I'm not concerned about the lifetime management of collection. The caller should expect this to keep a reference to the object.

I saw in your tests that you were using integers and uint256 as crazy delimiters. But I think just supporting strings is enough (if you have a crazy delimiter, convert it to a string first). Here's a patch if you want to see what this looks like: seelabs@2bf7647

seelabs

👍 I'm approving this now, but I did leave a comment on forcing the delim to be an rvalue ref. I'd leave that as a value parameter to keep the interface easier to use.

seelabs · 2022-02-28T14:32:33Z

src/ripple/basics/join.h

@@ -44,7 +42,7 @@ class CollectionAndDelimiter
    Collection const& collection;
    std::string const delimiter;

-    explicit CollectionAndDelimiter(Collection const& c, std::string delim)
+    explicit CollectionAndDelimiter(Collection const& c, std::string&& delim)


I would probably leave delim as a value, not an rvalue (here and in cases below). The common use case of passing a cstring will act the same. It will act differently if someone already has a delim as a string that they want to pass in. Making this a rvalue might have a performance benefit - it forces the caller to decide if they need to copy the delimiter or if they can just move it. But it also make the interface a little harder to use. For a class like this I'd choose to make the interface easier to use.

I'm fine with it as-is too. But thought I'd make the note.

Yeah, that makes sense. I'll change it back.

ximinez · 2022-03-24T23:05:34Z

Marked "passed" and squashed.

ximinez changed the title ~~Memory~~ Incremental improvements to path finding memory usage Feb 12, 2022

ximinez requested a review from mtrippled February 12, 2022 01:15

ximinez assigned mtrippled Feb 12, 2022

ximinez requested a review from seelabs February 12, 2022 01:15

ximinez assigned seelabs Feb 12, 2022

seelabs reviewed Feb 15, 2022

View reviewed changes

ximinez force-pushed the memory branch from 3f78992 to 2be56a2 Compare February 16, 2022 21:07

mtrippled approved these changes Feb 21, 2022

View reviewed changes

seelabs reviewed Feb 25, 2022

View reviewed changes

seelabs approved these changes Feb 28, 2022

View reviewed changes

ximinez mentioned this pull request Mar 1, 2022

Further incremental improvements to path finding memory usage #4111

Closed

ximinez force-pushed the memory branch 3 times, most recently from 911f994 to 42a3704 Compare March 15, 2022 22:13

ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Mar 24, 2022

ximinez force-pushed the memory branch from 42a3704 to d55e235 Compare March 24, 2022 23:05

ximinez closed this Apr 1, 2022

ximinez force-pushed the memory branch from d55e235 to 711608e Compare April 1, 2022 17:24

ximinez deleted the memory branch April 1, 2022 17:37

ximinez mentioned this pull request Apr 1, 2022

1.9.0-b2 #4127

Merged

ximinez mentioned this pull request Nov 20, 2023

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incremental improvements to path finding memory usage #4099

Incremental improvements to path finding memory usage #4099

ximinez commented Feb 12, 2022 •

edited

Loading

seelabs left a comment

seelabs Feb 15, 2022

seelabs Feb 15, 2022

ximinez Feb 16, 2022

seelabs Feb 15, 2022

ximinez Feb 16, 2022

seelabs Feb 15, 2022

ximinez Feb 16, 2022

seelabs Feb 15, 2022 •

edited

Loading

ximinez Feb 16, 2022

mtrippled left a comment

seelabs left a comment

seelabs Feb 25, 2022

seelabs left a comment

seelabs Feb 28, 2022 •

edited

Loading

ximinez Mar 1, 2022

ximinez commented Mar 24, 2022

		@@ -80,15 +90,28 @@ PathRequests::updateAll(

		int processed = 0, removed = 0;

		auto getSubscriber = [](PathRequest::pointer& request) -> InfoSub::pointer {

Incremental improvements to path finding memory usage #4099

Incremental improvements to path finding memory usage #4099

Conversation

ximinez commented Feb 12, 2022 • edited Loading

High Level Overview of Change

Type of Change

Test Plan

Future Tasks

seelabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs Feb 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrippled left a comment

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seelabs left a comment

Choose a reason for hiding this comment

seelabs Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ximinez commented Mar 24, 2022

ximinez commented Feb 12, 2022 •

edited

Loading

seelabs Feb 15, 2022 •

edited

Loading

seelabs Feb 28, 2022 •

edited

Loading