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

Mempool multifetch #17139

Merged
merged 3 commits into from Dec 22, 2023
Merged

Mempool multifetch #17139

merged 3 commits into from Dec 22, 2023

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Dec 21, 2023

This PR is best reviewed one commit at a time. But the second commit contains the bulk of the changes.

checkpoint merge

Reverse this patch when merging to main:

diff --git a/tests/core/mempool/test_mempool_manager.py b/tests/core/mempool/test_mempool_manager.py
index ce187fe7f..7a7d32899 100644
--- a/tests/core/mempool/test_mempool_manager.py
+++ b/tests/core/mempool/test_mempool_manager.py
@@ -1753,8 +1753,8 @@ async def test_mempool_timelocks(cond1: List[object], cond2: List[object], expec
     )
 
     coin_spends = [
-        make_spend(coins[0], IDENTITY_PUZZLE, Program.to([cond1])),
-        make_spend(coins[1], IDENTITY_PUZZLE, Program.to([cond2])),
+        CoinSpend(coins[0], IDENTITY_PUZZLE, Program.to([cond1])),
+        CoinSpend(coins[1], IDENTITY_PUZZLE, Program.to([cond2])),
     ]
 
     bundle = SpendBundle(coin_spends, G2Element())

Purpose:

This patch improves the disk I/O performance of the mempool by fetching coin records in batches instead of one at a time. It primarily reduces CPU usage when adding spend bundles to the mempool, which is especially critical when updating the mempool through the new-block slow-path.

There are two key changes in this PR:

get_coin_records

The MempoolManager and Mempool objects are passed a function from the CoinStore, get_coin_record, that looks up a single coin record. The first change is to instead of pass in get_coin_records. This function fetches multiple coin records in a single query.

This change requires updates a lot of tests, which ends up being the build of the changes. The core changes, however, are in def validate_spend_bundle() in mempool_manager.py.

new-peak slow-path

The second change (3rd commit) is to, when entering the slow-path to update the peak; fetch all spent coins in a single query, stick the CoinRecords in a dictionary. Pass in a local version of get_coin_records that only looks up coins from the local dictionary to add_spend_bundle(), when adding all the transactions into the new mempool.

This turns thousands of individual CoinRecord lookups into a single query.

Current Behavior:

Mempool takes a function that can fetch one CoinRecord. Coin record are looked up one at a time when validating transactions.

New Behavior:

Mempool takes a function that can fetch multiple CoinRecords. Coin record are looked up in a single query.
When updating the peak through the slow-path, all coin spends are looked up in a single query, to speed up re-adding the transactions to the new mempool instance.

Testing Notes:

This was tested on IPS machines. The wall-clock time was not improved as much as the CPU usage:

patch mempool size wall-clock time time per spend
before 3551 44.42s 12.5ms
after 7069 67.71 9.57ms

Before

chia-hotspot-3532-3574

After

chia-hotspot-13804-13861

@arvidn arvidn added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Dec 21, 2023
Copy link

coveralls-official bot commented Dec 21, 2023

Pull Request Test Coverage Report for Build 7304023557

  • 92 of 92 (100.0%) changed or added relevant lines in 6 files are covered.
  • 16 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.002%) to 90.465%

Files with Coverage Reduction New Missed Lines %
chia/rpc/rpc_server.py 1 88.36%
chia/rpc/util.py 1 93.22%
chia/simulator/setup_nodes.py 1 98.31%
chia/server/node_discovery.py 4 79.55%
chia/full_node/full_node.py 9 84.6%
Totals Coverage Status
Change from base Build 7294432788: -0.002%
Covered Lines: 93907
Relevant Lines: 103760

💛 - Coveralls

@arvidn arvidn marked this pull request as ready for review December 22, 2023 17:56
@arvidn arvidn requested a review from a team as a code owner December 22, 2023 17:56
wjblanke
wjblanke previously approved these changes Dec 22, 2023
Copy link
Contributor

@wjblanke wjblanke left a comment

Choose a reason for hiding this comment

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

aok

@github-actions github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label Dec 22, 2023
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@arvidn arvidn changed the base branch from main to release/2.1.4 December 22, 2023 21:12
@arvidn arvidn requested a review from wjblanke December 22, 2023 21:12
@arvidn arvidn closed this Dec 22, 2023
@arvidn arvidn reopened this Dec 22, 2023
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Dec 22, 2023
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

wjblanke
wjblanke previously approved these changes Dec 22, 2023
Copy link
Contributor

@wjblanke wjblanke left a comment

Choose a reason for hiding this comment

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

aok

@cmmarslender cmmarslender merged commit 507899f into release/2.1.4 Dec 22, 2023
581 checks passed
@cmmarslender cmmarslender deleted the mempool-multifetch branch December 22, 2023 22:29
mikehw pushed a commit to mikehw/chia-blockchain that referenced this pull request Jan 11, 2024
* update type annotation for CoinStore.get_coin_records to support both List and Set

* update the mempool to fetch multiple coin records per query

* optimize the slow-path of updating the mempool by fetching all coin records up-front, in a single sql query
mikehw pushed a commit to mikehw/chia-blockchain that referenced this pull request Jan 11, 2024
* update type annotation for CoinStore.get_coin_records to support both List and Set

* update the mempool to fetch multiple coin records per query

* optimize the slow-path of updating the mempool by fetching all coin records up-front, in a single sql query
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changed Required label for PR that categorizes merge commit message as "Changed" for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants