test: refactor use of getrawmempool in functional tests for efficiency#22707
Conversation
glozow
left a comment
glozow
left a comment
There was a problem hiding this comment.
Concept ACK, it certainly saves time to just fetch the entry we need instead of the entire mempool, particularly because this test puts a lot of transactions in there. It is significantly faster on my machine as well.
Should do this for other functional tests as well, e.g:
test/functional/mempool_compatibility.py
test/functional/rpc_fundrawtransaction.py
test/functional/mempool_package_limits.py
(From a quick grep for getrawmempool(True) and getrawmempool(verbose=True) and eliminating the ones that should actually use getrawmempool)
|
Thanks @glozow - good suggestions, I'll follow up with these shortly |
mjdietzx
force-pushed
the
test_improve_mempool_updatefromblock_efficiency
branch
from
99fb430 to
47c48b5
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
There was a problem hiding this comment.
Code Review and Concept ACK
+1 for always preferring getmempoolentry over getrawmempool when you are referencing specific txns.
left a suggestion about using deprecated fee fields, but like i mentioned, it might be out of scope for this PR.
i would also suggest updating the title and description of the PR to reflect that you are globally replacing references to getrawmempool (right now it's still specific to mempool_updatefrom)
after updating the title and description, 86dbd54 and 7734971 should be squashed
you could even squash all three, since both changes fall under refactoring for efficiency 🤷♂️
mjdietzx
changed the title
|
Thanks for the review @josibake ! Updated PR title/description accordingly
I think this is/should be done in your PR #22689. If we have merge conflicts one of us will just need to rebase depending on whose PR gets merged first. |
|
Concept ACK |
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
Tested ACK 47c48b5
On my machine, the speed-up of mempool_updatefromblock.py is quite impressive (>2x):
master branch:
$ time ./test/functional/mempool_updatefromblock.py
3m18.16s real 2m04.55s user 0m33.54s system
PR branch:
$ time ./test/functional/mempool_updatefromblock.py
1m29.11s real 0m34.37s user 0m14.79s system
7 participants
I don't think this changes the intention of the test. But it does shave ~30 seconds off the time it takes to run. From what I've seen our CI
macOS 11 native [gui] [no depends]runsmempool_updatefrom.pyin ~135 seconds. After this PR it should run in ~105 secondsI noticed this improvement should probably be made when testing performance/runtimes of #22698. But I wanted to separate this out from that PR so the affects of each is decoupled
Edit: The major change in this PR is improving mempool_updatefrom.py's runtime as this is a very long running test. Then made the same efficiency improvements across all the functional tests as it made since to do that here