Add new mode to apply load for tx set validation - #5404
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an optional apply-load benchmark path covering tx-set decoding, validation, local consensus, and ledger application.
Changes:
- Adds configurable validation-and-apply timing.
- Records phase timings and signature-cache metrics.
- Documents the mode and new validation metric.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/simulation/ApplyLoad.h |
Declares timing paths and phase statistics. |
src/simulation/ApplyLoad.cpp |
Implements consensus-backed benchmarking. |
src/main/Config.h |
Defines timing configuration. |
src/main/Config.cpp |
Parses timing configuration. |
src/herder/HerderSCPDriver.h |
Declares validation timer. |
src/herder/HerderSCPDriver.cpp |
Instruments tx-set validation. |
docs/software/commands.md |
Documents apply-load timing paths. |
docs/metrics.md |
Documents the validation metric. |
docs/apply-load-benchmark-sac.cfg |
Adds example timing configuration. |
| if (measuresTxSetValidation()) | ||
| { | ||
| closeLedgerViaConsensus(txs, recordUtilization); |
There was a problem hiding this comment.
Agree, we can just add a GENERATE for the modes in the existing acceptance tests.
| synthetic transactions. The benchmark omits the overlay and mempool, but | ||
| each iteration reconstructs the tx set from serialized bytes and runs real | ||
| consensus with the node as its own single-validator quorum. It does not | ||
| simulate network transport, peer fetching, or multi-node timing. |
There was a problem hiding this comment.
I think we can just say that it may measure txset stuff
| APPLY_LOAD_MODE="benchmark" | ||
| APPLY_LOAD_MODEL_TX="sac" | ||
|
|
||
| # Which timing path to use: "apply" preserves the historical apply-only |
There was a problem hiding this comment.
nit: I don't think it's correct to call the apply-only mode 'historical', both have a valid use case
| synthetic transactions. The benchmark omits the overlay and mempool, but | ||
| each iteration reconstructs the tx set from serialized bytes and runs real | ||
| consensus with the node as its own single-validator quorum. It does not | ||
| simulate network transport, peer fetching, or multi-node timing. |
There was a problem hiding this comment.
I think we can just say that it may measure txset stuff
| if (pRes == nullptr) | ||
| { | ||
| std::string zoneTxt("miss"); | ||
| ZoneText(zoneTxt.c_str(), zoneTxt.size()); |
There was a problem hiding this comment.
nit: You should be able to use one-line ZoneNamed here, as your string is static (same for 'hit' branch)
|
|
||
| private: | ||
| bool | ||
| measuresTxSetValidation() const |
There was a problem hiding this comment.
nit: Could you please move this implementation into cpp file as well?
| # Which timing path to use: "apply" preserves the historical apply-only | ||
| # benchmark, while "txset-validation-and-apply" simulates a non-leader receiving | ||
| # and validating a tx set before applying it. Tx-set creation is not measured. | ||
| APPLY_LOAD_TIMING_PHASES = "apply" |
There was a problem hiding this comment.
We need to add this to all the benchmark configs
| } | ||
|
|
||
| void | ||
| ApplyLoad::logPhaseStats(std::string const& label, |
There was a problem hiding this comment.
This doesn't need to be a member function
| mPhaseEndToEndMs.end(), 0.0); | ||
| if (e2eSum > 0.0) | ||
| { | ||
| CLOG_WARNING(Perf, "txset validation share of end-to-end: {:.2f}%", |
There was a problem hiding this comment.
Wouldn't it make more sense to compute statistics on ratios, instead of just outputting effectively the ratio of means? I think understanding the variance in share is useful, and we're hiding it here.
| if (mMode == ApplyLoadMode::MAX_SAC_TPS) | ||
| { | ||
|
|
||
| if (measuresTxSetValidation()) |
There was a problem hiding this comment.
Most of the gates on measuresTxSetValidation are kind of brittle as they're missing the actual intention of the check and sometimes do not exhaust all the options explicitly when they should.
I think we could future-proof the code a bit and add more targeted checks: e.g. here we're specifically interested in mode not being apply, so we should update the check and message to mode != APPLY. Another example: closeBenchmarkLedger should probably use an exhaustive switch instead of an if. logConfiguredPhaseStats actually doesn't log anything for the apply-only mode, even though from the name one would assume that it does. Basically we should gate the code semantically instead of a blanket check on one of the enum variants.
I realize this is kind of minor for now, but it's very easy for apply load to grow unwieldy (and it's already pretty complex), so I'd like to start with more robust code in the first place.
| if (measuresTxSetValidation()) | ||
| { | ||
| closeLedgerViaConsensus(txs, recordUtilization); |
There was a problem hiding this comment.
Agree, we can just add a GENERATE for the modes in the existing acceptance tests.
Description
This adds a new mode to apply load, allowing us for more "end to end" simulations. In the new mode, we time and simulate ballot phase through the end of apply, but in a single node network such that SCP state transitions occur instantly. This is useful, as it allows us to benchmark the processing overhead associated with consensus, as well as measure cache performance and dedup similar work done during tx set validation and the later application of this transaction set.
The motivation for this test was from my recent block latency experiments. I noticed how optimizations in network calls, reducing bandwidth, etc were not moving the needle on actual block latency. It turned out that consensus latency was greatly affected by CPU based bottlenecks, not just network calls. On supercluster, it's challenging to benchmark CPU costs of concensus, as you can't easily run tracy or profilers and have limited visibility to single node performance. This test allows us to much more easily measure and improve the processing heavy aspects of concensus. In overlay-v2-shared for a block with 6K SAC transfers, we saw about 300 ms of non-network "ingestion" time when receiving a tx set and 500 ms of apply. Imo this was a significant blind spot in our single node apply load tests previously.
Note that the actual number produced by this test is meaningless, as consensus without a network tells us very little about overall performance. However, the reported phase timings are very useful in identifying bottlenecks and comparing solutions. This simulates the processing done by a non-leader node. In the future, it may also be helpful to add a mode for block construction as well.
Checklist
clang-formatv8.0.0 (viamake formator the Visual Studio extension)