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

(Reverted by #4882) Asynchronously write batches to NuDB. #4503

Merged
merged 2 commits into from
Sep 11, 2023

Conversation

mtrippled
Copy link
Collaborator

@mtrippled mtrippled commented Apr 21, 2023

High Level Overview of Change

For NuDB, use BatchWriter for all writes. This is identical behavior to that of RocksDB.
Increase batch size.

This PR is related to 2 others that, when combined, increase throughput significantly:
#4504
#4505

Context of Change

This improves transaction throughput. The bulk of writes to the nodestore happen during the "accept" phase of consensus as a new ledger is built. The number of objects increases directly with transaction volume, and write latency directly affects consensus interval duration. Batching writes and performing them in the background minimizes this impact. The new behavior is identical to that of RocksDB, which has used BatchWriter in production for several years already. Increasing the batch size should allow all ledgers for the forseeable future to be persisted this way without blocking consensus.

Type of Change

  • [X ] New feature (non-breaking change which adds functionality)

This change is part of a suite of throughput-enhancing pull requests. They have been tested in conjunction with one another.

@intelliot
Copy link
Collaborator

note: Howard and John will try to look this week

Copy link
Contributor

@HowardHinnant HowardHinnant left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Collaborator

@ximinez ximinez left a comment

Choose a reason for hiding this comment

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

LGTM

@intelliot intelliot added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jun 20, 2023
@intelliot
Copy link
Collaborator

intelliot commented Jun 30, 2023

note: the current plan is for this to be squashed and merged together with the other 2 PRs in this milestone, after all of them have been approved in code review (by 2+ reviewers each).

@intelliot
Copy link
Collaborator

correction: the 3 PRs will be merged at about the same time, but they will be 3 separate commits on develop.

manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
@manojsdoshi manojsdoshi mentioned this pull request Aug 18, 2023
manojsdoshi pushed a commit to manojsdoshi/rippled that referenced this pull request Aug 18, 2023
intelliot added a commit that referenced this pull request Aug 30, 2023
@intelliot intelliot reopened this Sep 1, 2023
@intelliot
Copy link
Collaborator

Since this was reverted in 1.12.0-rc3, this PR is being reopened for additional review, testing, and potential improvements, with the expectation that it will be merged again soon (e.g. for the 1.13 or 2.0 release).

mtrippled added a commit to mtrippled/rippled that referenced this pull request Sep 4, 2023
@intelliot
Copy link
Collaborator

note: still prefer to merge this at the same time as #4504 #4505

@intelliot intelliot merged commit 1d9db1b into XRPLF:develop Sep 11, 2023
30 checks passed
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 25, 2023
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 25, 2023
ckeshava pushed a commit to ckeshava/rippled that referenced this pull request Sep 25, 2023
mtrippled added a commit to mtrippled/rippled that referenced this pull request Jan 11, 2024
mtrippled added a commit to mtrippled/rippled that referenced this pull request Jan 16, 2024
@intelliot
Copy link
Collaborator

Note: This PR introduces a bug which #4762 was expected to fix, but it apparently does not. Thus, the proposal of #4882 which just reverts #4503.

intelliot pushed a commit that referenced this pull request Jan 19, 2024
This reverts commit 1d9db1b.

This improves the stability of online deletion.
@JoelKatz
Copy link
Collaborator

Having looked at this code and looked at its effects, I think it should be reverted if it hasn't been already.

My vague recollection was that the batch writing code was added because earlier databases had a habit of stalling thread that called in to write even under low load. If load gets too high, there is no choice but to impose a delay on each write because otherwise data to be written will back up in memory without limit. This code imposes a limit on how much data can back up in memory and then it stalls writer threads.

However, NuDB has built in logic to do exactly this already. And it does it much better than this code can because NuDB does it in a way that is both aware of, and connected to, the database internals.

For example, NuDB knows precisely what its write capabilities are and can throttle only if the write rate exceeds its capabilities. The batch writer just has a fixed buffer threshold. NuDB's write logic drops redundant writes without ever buffering them or risking delaying a thread because of them. The batch writer doesn't do that and so can hold multiple copies of the same object. And NuDB adds delayed writes to a cache that will allow the data to be fetched immediately. The batch writer doesn't do that and so may trigger extra work to fetch the same object twice.

If there is a demonstrated need to reduce the amount of writes delayed, the better option is to increase NuDB's burst size which is already a configurable option. Even better might be a change to NuDB to allow the burst size to be raised automatically, say, to the lesser of its configured value or half the size of the previous write batch.

@intelliot
Copy link
Collaborator

This has already been reverted by #4882, but thank you for looking into this, because otherwise we were considering re-merging this again (in conjunction with #4762).

@intelliot intelliot changed the title Asynchronously write batches to NuDB. (Reverted by #4882) Asynchronously write batches to NuDB. Jan 22, 2024
@intelliot intelliot added Perf Test Desired (Optional) RippleX Perf Team should look at this PR. The PR will not necessarily wait for testing to finish and removed Performance/Resource Improvement labels Jan 25, 2024
sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
This reverts commit 1d9db1b.

This improves the stability of online deletion.
@intelliot intelliot added the Reverted Changes which should still be considered for re-merging. See "Closed" PRs with this label label Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Perf Test Desired (Optional) RippleX Perf Team should look at this PR. The PR will not necessarily wait for testing to finish Reverted Changes which should still be considered for re-merging. See "Closed" PRs with this label
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants