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

Further incremental improvements to path finding memory usage #4111

Closed
wants to merge 4 commits into from

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Mar 1, 2022

High Level Overview of Change

These changes continue building on top of PR #4099, which in turn builds on the changes from PR #4080. Both of those PRs are approved to merge. Thus, reviewers can skip straight to the commit labelled, "Don't load trust lines that can't participate in path finding".

Context of Change

As described in the other two PRs, the number of trustlines on the network has grown significantly over the last few months. That in-turn has led to a significant increase in the amount of memory used to load and cache those trustlines.

Type of Change

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

Before / After

These changes take advantage of two facts:

  1. "A path is considered invalid if and only if it enters and exits an address node through trust lines where No Ripple has been enabled for that address." (https://xrpl.org/rippling.html#specifics)
  2. Most "end-user" accounts / wallets do not have the "default ripple" flag set, and thus have the NoRipple flag set on all of their trustlines.

Before

Currently, during path finding, all trust lines which might link the source account to the destination account are examined and cached. The path finding engine checks for invalid paths ("enters and exits an address node through trust lines where No Ripple has been enabled"), and does not use any of the cached trust lines to look for more paths, but the trust lines remain cached, potentially for a rather long time.

After

After these changes, during path finding, the trust line lookups for a given account consider the state of the no ripple flag on that account's side of the trust line used to find that account. If not set (rippling is allowed), the account is considered "outgoing", and all trust lines are loaded and cached. However, if the no ripple flag is set (rippling is not allowed), the account is considered "incoming", and only trust lines which do not have the no ripple flag set (thus allowing rippling) are loaded and cached.

Example

Consider the case where 1000 wallets all have trustlines to the same 100 issued tokens. Alice and Bob are among those 1000. If Alice tries to find a path to Bob, with the current behavior: Alice's 100 trust lines will be loaded and cached. The first token line will be considered, leading to the token issuer. The token issuer's 1000 trust lines will be loaded and cached. Then for each of those 999 other wallet accounts, the 100 trust lines to each issuer will be loaded and cached. Unfortunately, none of those trust lines will be usable in a path. This effectively leads to 99,900 unusable cached trust lines. In this example, the second token issuer will lead to the same set of 1000 accounts, but on the mainnet, there are more accounts trusting more tokens without necessarily overlapping, leading to even bigger data sets.

With these changes, each time a wallet account is considered, no trust lines will be loaded or cached, preventing those 99,900 unusable trustlines from taking up memory. (Under the hood, the SLE will be loaded regardless, but no PathFindTrustLines will be created. In the case where there are 0 usable trust lines found, the std::vector will not even be cached.)

Additionally, the changes handle the case where an account has some trust lines with the no ripple flag and without, and avoids duplication.

Test Plan

This test plan is similar to the one for #4099

Run an expensive path_find request. Observe that less memory is used by the rippled process overall. Using the get_counts command, observe that fewer instances of PathFindTrustLine are being created than were created for #4099 and older. (Note that the counter for PathFindTrustLine was only added in #4080.)

@ximinez ximinez requested review from seelabs and mtrippled March 1, 2022 21:36
@ximinez ximinez force-pushed the memory3 branch 2 times, most recently from 073b687 to 4bff92f Compare March 15, 2022 22:49
Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice optimization! I like this a lot.

Since we're filtering out trustlines, we should make sure there are tests that have an incoming trustline set to ripple and an outgoing trusline set to no ripple that tests whether the pathfinder still finds that path. If we don't have such a test can we add one?

auto& rippleLines = lrCache->getRippleLines(account);

for (auto const& item : rippleLines)
if (auto const lines = lrCache->getRippleLines(account, true))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the true param makes this line harder to understand. I'd either add an inline comment getRippleLines(account, /*outgoing*/true), make a named variable, or make the parameter an enum. I probably vote for an enum, but whatever you decide is fine with me. (ditto for the other calls to this function).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ximinez
Copy link
Collaborator Author

ximinez commented Mar 22, 2022

Since we're filtering out trustlines, we should make sure there are tests that have an incoming trustline set to ripple and an outgoing trusline set to no ripple that tests whether the pathfinder still finds that path. If we don't have such a test can we add one?

I just pushed a commit that adds a test case to test all four combinations of the incoming and outgoing flags.

Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Great job on this!

@manojsdoshi manojsdoshi mentioned this pull request Mar 30, 2022
@ximinez ximinez deleted the memory3 branch March 31, 2022 18:48
@ximinez ximinez restored the memory3 branch March 31, 2022 18:49
@ximinez ximinez reopened this Apr 1, 2022
@ximinez
Copy link
Collaborator Author

ximinez commented Apr 4, 2022

This PR was incorrectly closed by #4127, so I reopened it.

ximinez added 4 commits April 11, 2022 14:41
* "A path is considered invalid if and only if it enters and exits an
  address node through trust lines where No Ripple has been enabled for
  that address." (https://xrpl.org/rippling.html#specifics)
* When loading trust lines for an account "Alice" which was reached
  via a trust line that has the No Ripple flag set on Alice's side, do
  not use or cache any of Alice's trust lines which have the No Ripple
  flag set on Alice's side. For typical "end-user" accounts, this will
  return no trust lines.
* Also remove an unused variable that's giving VS a problem
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants