bench: Add OrphanTxPool benchmark #19377
Conversation
|
utACK 059c8e6 |
|
@hebasto can you provide motivation for this change? In practice, only one orphan transactions is ever evicted at a time (in |
|
I'm a -0.5 on this. I don't think it's worth adding a microbench for a different usage pattern than is used in the product, and for something that is always going to be quick. |
|
microbenchmarks are never going to match the exact usage pattern in the end product. This is true for all existing microbenchmarks (like the mempool eviction one, etc ...). Absent the details, I think having a benchmark is slightly preferable to not having a benchmark because it might catch accidental performance regressions. And since we don't have any user-facing compatibility concerns for tests in general, adding and removing tests can be done much more easily than (let's say) an RPC method. In general I think that the cost of a pull request that adds a test that passes can only be negative or null (when it is testing nothing). And it should always be an uncontroversial option to remove a not-too-useful test that is in the way of other changes/refactoring/features. I haven't looked at the specifics of this pull request, but I think in general we need more benchmarks (or fuzzers) for p2p tx relay, as that is the primary DoS attack vector on the live network. |
|
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. |
I'm sorry - this is wrong. All code added to the repo has a cost - in maintenance, in review, in discussion as to whether to remove it later. Tests/benches should only be added if they're useful. |
src/net_processing.cpp
Outdated
| LOCK(g_cs_orphans); | ||
| SweepOutExpiredOrphans(); | ||
| return EvictOrphanTxs(nMaxOrphans); | ||
| } |
There was a problem hiding this comment.
Why is net_processing changed in a pull that adds a benchmark? Seems unnecessary to me
There was a problem hiding this comment.
To benchmark EvictOrphanTxs() without sweeping out functionality of the current LimitOrphanTxSize().
What could you suggest to change?
There was a problem hiding this comment.
If you explicitly want to disable the time based eviction, it could be done with mocktime in the benchmark, no?
|
Updated 059c8e6 -> 2ee8c0b (pr19377.01 -> pr19377.02, diff):
|
|
Rebased 2ee8c0b -> b0a5761 (pr19377.02 -> pr19377.03) due to the conflict with #18011. |
| } | ||
|
|
||
| bench.batch(txs.size()).unit("byte").run([&] { | ||
| WITH_LOCK(g_cs_orphans, for (const auto& tx : txs) { AddOrphanTx(tx, 0); }); |
There was a problem hiding this comment.
Concept ACK. Had a look as this "conflicts" with #22284, rebased it to current master and began building; it needs updating here and line 17 above:
bench/orphan_tx_pool.cpp:17:83: error: use of undeclared identifier 'g_cs_orphans'
bool AddOrphanTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans);
^
bench/orphan_tx_pool.cpp:37:19: error: use of undeclared identifier 'g_cs_orphans'
WITH_LOCK(g_cs_orphans, for (const auto& tx : txs) { AddOrphanTx(tx, 0); });
^
bench/orphan_tx_pool.cpp:37:19: error: use of undeclared identifier 'g_cs_orphans'
3 errors generated.|
Marked this as a "draft" for now. |
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
This PR adds a new benchmark for adding/evicting orphan transactions to/from the
mapOrphanTransactions.This benchmark could be useful while considering relevant code changes (e.g., #19374) or changing the
DEFAULT_MAX_ORPHAN_TRANSACTIONSparameter.The output example: