bench: Expand mempool eviction benchmarking #27019
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process. |
maflcko
changed the title
instagibbs
force-pushed
the
bench_eviction
branch
from
1fed1e3 to
4d5a2ee
Compare
|
Sorry for the drive-by suggestion: from the title, I thought this PR might be about the peer eviction benchmarks. Maybe update the title to s/eviction/mempool eviction/. |
instagibbs
changed the title
|
@jonatack good point, updated |
instagibbs
force-pushed
the
bench_eviction
branch
from
396216d to
a6fc02b
Compare
|
fixups included, squashed |
| CMutableTransaction tx = CMutableTransaction(); | ||
| tx.vin.resize(num_puts); | ||
| tx.vout.resize(num_puts); | ||
| for (size_t put_index=0; put_index<num_puts; put_index++) { |
There was a problem hiding this comment.
Can you apply the changes to the other loops too? i.e.
| for (size_t put_index=0; put_index<num_puts; put_index++) { | |
| for (size_t put_index{0}; put_index < num_puts; ++put_index) { |
| if (put_index < txn_index) { | ||
| // We spend put_index's txn's in the next available slot for each previous transaction | ||
| assert(put_index + txn_index >= 1); | ||
| tx.vin[put_index].prevout = COutPoint(txns[put_index]->GetHash(), put_index + txn_index - 1); |
There was a problem hiding this comment.
Wait, if you're using txns[put_index] and you're using the same txns reference each time you call add_parents_child, aren't you having all subsequent packages spend outputs from txns[0:24] too? That means all these packages conflict with each other.
You could fix this by adding set_num * package_size or something to the index. But what would probably make a better interface is if add_parents_child returns the list of transactions it creates, and you append them to your larger list of packages.
There was a problem hiding this comment.
Hmmm, traced out a package size of 4, looks like I'm referencing non-existent outputs, not re-using them(maybe both :) ).
Clearly this is confusing and broken either way.
To take a step back, after doing this draft I realized https://github.com/bitcoin/bitcoin/blob/master/src/bench/mempool_stress.cpp exists. I'm fine if this doesn't get merged if it doesn't add anything new? Helped me learn about the benchmarks if nothing else. Thoughts?
There was a problem hiding this comment.
Ah true, I suppose ComplexMemPool is already benching eviction of packages. Fine with closing. The effort is appreciated 🙏
There was a problem hiding this comment.
I could change the PR to remove this silly bench.. :P
5 participants
Took the existing benchmark and made is something more exhaustive, targeting eviction of 2500 transactions of different shapes. of dependencies.
Magic number comes from: 100 children being RBF'd, each bumping 24 parents = 2500 transactions
Turns out the benchmark
ComplexMemPoolalready does something similar, so if we don't want to expand it, maybe just remove it in favor of the existing better benchmark?