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

improve performance of total_mempool_fees() and total_mempool_cost() #17107

Merged
merged 4 commits into from Dec 22, 2023

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Dec 20, 2023

Purpose:

Currently, the mempool asks sqlite what the total cost and total fees are of the items in the mempool. This is very reliable as all the book-keeping is done by sqlite. However, it's also slow. It appears sqlite does not maintain these sums continuously, but actually computes them every time we ask.

This patch adds "manual" book-keeping of the total cost and total fees in the mempool, avoiding a SQL query every time we want to know.

Current Behavior:

total_mempool_fees() and total_mempool_cost() are expensive calls.

New Behavior:

total_mempool_fees() and total_mempool_cost() are cheap calls.

profiles

benchmark before after after/before
add_spend_bundle() 106.4s 58.5s 55%
add_spend_bundle() with replace-by-fee 198.1s 60.9s 30.76%

Before

mempool-add-st

After

mempool-add-st

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

coveralls-official bot commented Dec 20, 2023

Pull Request Test Coverage Report for Build 7290982749

  • 65 of 65 (100.0%) changed or added relevant lines in 5 files are covered.
  • 30 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.1%) to 90.552%

Files with Coverage Reduction New Missed Lines %
chia/timelord/timelord_launcher.py 1 69.77%
tests/core/daemon/test_daemon.py 3 99.42%
chia/server/node_discovery.py 4 78.35%
tests/core/util/test_lockfile.py 22 77.73%
Totals Coverage Status
Change from base Build 7283399998: 0.1%
Covered Lines: 93991
Relevant Lines: 103767

💛 - Coveralls

@arvidn arvidn marked this pull request as ready for review December 20, 2023 19:28
@arvidn arvidn requested a review from a team as a code owner December 20, 2023 19:28
@neurosis69
Copy link
Contributor

It may be worth to run the profiler with these 3 pragma changes as well, if you haven't tried them.

cache_size
What:  Change sqlites cache size from the default 2 MiB to 200 MiB
Why:   Suggested a random number just to find out if a bigger sqlite cache would improve code. The required size can than probably be calculated.

journal_mode
What:  Change sqlites journal_mode from the default MEMORY to OFF.
Why:   I didn't find any explicit rollbacks/commits in the code and my understanding of this mempool is, that it gets feed from the peers as soon as it is started up -> likely no journal required?

temp_store
What:  Change sqlites temp_store from the default 0, which means file, to 2, which means memory.
Why:   My understanding is, that in theory even if an in-memory sqlite database is used and as long as this parameter is set to 0, the indexes aren't stored in memory. Doesn't sound reasonable but probably worth to verify if it has an impact imho.

    def __init__(self, mempool_info: MempoolInfo, fee_estimator: FeeEstimatorInterface):
        self._db_conn = sqlite3.connect(":memory:")
        self._items = {}
        self._block_height = uint32(0)
        self._timestamp = uint64(0)

        with self._db_conn:
            self._db_conn.execute("PRAGMA cache_size = -200000;")
            self._db_conn.execute("PRAGMA journal_mode = OFF;")
            self._db_conn.execute("PRAGMA temp_store = 2;")

@arvidn
Copy link
Contributor Author

arvidn commented Dec 20, 2023

@neurosis69 the mempool's sqlite database is ":memory:" already. I wouldn't expect any of those pragmas to make a difference

@arvidn
Copy link
Contributor Author

arvidn commented Dec 20, 2023

my understanding is that a :memory: database can only have a single connection, so it's already "single threaded" so to speak. Since it's in memory, there's no fault tolerance required, no journaling. Would sqlite really enable such redundant features by default for an in-memory database?

@neurosis69
Copy link
Contributor

I'm with you, it doesn't sound reasonable but there are still some hints in the documentation, that's why I asked.
Don't have much time myself to set up a testing environment quickly, but I thought you may be able to quickly execute some runs and verify it.

For instance, this info is from the journal_mode parameter:

Note that the journal_mode for an [in-memory database](https://www.sqlite.org/inmemorydb.html) is either MEMORY or OFF and can not be changed to a different value. An attempt to change the journal_mode of an [in-memory database](https://www.sqlite.org/inmemorydb.html) to any setting other than MEMORY or OFF is ignored. Note also that the journal_mode cannot be changed while a transaction is active.

If it's safe to deactivate it I'd guess that a lots of sqlite internal code is skipped.

@emlowe
Copy link
Contributor

emlowe commented Dec 20, 2023

The OFF journal mode disables the atomic commit and rollback capabilities of SQLite. The ROLLBACK command is not available when OFF journal mode is set. And if a crash or power loss occurs in the middle of a transaction that uses the OFF journal mode, no recovery is possible and the database file will likely go corrupt. The MEMORY journal mode causes the rollback journal to be stored in memory rather than on disk. The ROLLBACK command still works when the journal mode is MEMORY, but because no file exists on disks for recovery, a crash or power loss in the middle of a transaction that uses the MEMORY journal mode will likely result in a corrupt database.

Copy link
Contributor

@AmineKhaldi AmineKhaldi 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, I think we're only missing accounting for (and covering) the case of removing conflicting transactions when adding a transaction.

chia/full_node/mempool.py Outdated Show resolved Hide resolved
chia/full_node/mempool.py Outdated Show resolved Hide resolved
chia/full_node/mempool.py Outdated Show resolved Hide resolved
chia/full_node/mempool.py Outdated Show resolved Hide resolved
chia/full_node/mempool_manager.py Show resolved Hide resolved
@arvidn
Copy link
Contributor Author

arvidn commented Dec 21, 2023

thanks for the review! I've updated it to move the accounting into remove_from_pool(), which should make it more reliable. Now I query the cost- and fee sums out of the table.

@arvidn arvidn force-pushed the mempool-performance branch 2 times, most recently from f56f79d to 0752d8c Compare December 21, 2023 16:29
@arvidn arvidn force-pushed the mempool-performance branch 2 times, most recently from d774255 to 3a0e4ce Compare December 21, 2023 16:53
AmineKhaldi
AmineKhaldi previously approved these changes Dec 21, 2023
@github-actions github-actions bot added merge_conflict Branch has conflicts that prevent merge to main labels Dec 21, 2023
Copy link
Contributor

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

1 similar comment
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 21, 2023 17:55
@AmineKhaldi AmineKhaldi reopened this Dec 21, 2023
@github-actions github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label Dec 21, 2023
Copy link
Contributor

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

@arvidn arvidn requested a review from wjblanke December 21, 2023 18:48
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

@arvidn arvidn added the ready_to_merge Submitter and reviewers think this is ready label Dec 21, 2023
@cmmarslender cmmarslender merged commit 55c064a into release/2.1.4 Dec 22, 2023
582 checks passed
@cmmarslender cmmarslender deleted the mempool-performance branch December 22, 2023 00:02
mikehw pushed a commit to mikehw/chia-blockchain that referenced this pull request Jan 11, 2024
…()` (Chia-Network#17107)

* improve performance of total_mempool_fees() and total_mempool_cost()

* log when updating the mempool using the slow-path

* extend test to ensure total-cost and total-fee book-keeping stays in sync with the DB

* lower mempool size from 50 blocks to 10 blocks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog ready_to_merge Submitter and reviewers think this is ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants