tests: speed up coinselector_tests #23338
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
cc @jnewbery |
Instead of using AddToWallet so that making a COutput will work, directly add the transaction into wallet.mapWallet. This bypasses many checks that AddToWallet will do which are pointless and just slow down this test.
Default to SQLiteDatabase instead of BerkeleyDatabase for CreateDummyWalletDatabase. Most tests already use descriptor wallets and the mock db doesn't really matter for tests. The tests where it does matter will make the db directly.
achow101
force-pushed
the
speed-up-cs-tests
branch
from
d00afd2 to
a52f1d1
Compare
glozow
reviewed
Oct 25, 2021
Member
glozow
left a comment
glozow
left a comment
There was a problem hiding this comment.
utACK a52f1d1
Since it's a coin selection unit test and add_coin's job is to populate the coins vector with UTXOs to select from, calling mapWallet.emplace() directly seems sensible. I don't know this code very well but AFAICT none of the coin selection functions rely on the something done in AddToWallet. On my machine, the test sped up from 33sec to 3sec.
Side note: the AddToWallet function declaration is missing a doxygen comment. It would be nice to add one.
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 25, 2021
a52f1d1 walletdb: Use SQLiteDatabase for mock wallet databases (Andrew Chow) a78c229 tests: Place into mapWallet in coinselector_tests (Andrew Chow) Pull request description: bitcoin#23288 changed coinselector_tests to use `DescriptorScriptPubKeyMan`, but it also ended up significantly slowing down the test, from 4 seconds to over 1 minute. It appears that the source of this slow down is with `CWallet::AddToWallet`, and primarily due to writing data to the mock wallet database. Because the only thing that is actually needed is for the created transaction to be placed into `CWallet::mapWallet`, this PR removes the call to `AddToWallet` and just places the transaction into `mapWallet` directly. This reduces the test time to 5 seconds. To speed things up further, `CreateMockWalletDatabase` is changed to make a `SQLiteDatabase` instead of a `BerkeleyDatabase`. This is safe because there are no tests that require a specific mock database type. ACKs for top commit: brunoerg: tACK a52f1d1 lsilva01: tACK a52f1d1. Performed 74.36% better on Ubuntu 20.04 (VM, 12 MB, 8vCPU). glozow: utACK a52f1d1 Tree-SHA512: da77936bfd2e816d2e71703567b9389d0ee79f3a4a690802ffe3469df5bed371b296cb822b897f625654dab9436d91fd6bc02364a518a47d746e487d70a72595
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#23288 changed coinselector_tests to use
DescriptorScriptPubKeyMan, but it also ended up significantly slowing down the test, from 4 seconds to over 1 minute. It appears that the source of this slow down is withCWallet::AddToWallet, and primarily due to writing data to the mock wallet database. Because the only thing that is actually needed is for the created transaction to be placed intoCWallet::mapWallet, this PR removes the call toAddToWalletand just places the transaction intomapWalletdirectly. This reduces the test time to 5 seconds.To speed things up further,
CreateMockWalletDatabaseis changed to make aSQLiteDatabaseinstead of aBerkeleyDatabase. This is safe because there are no tests that require a specific mock database type.