Add txset construction mode to apply load - #5448
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a leader-side txset-construction-and-apply benchmarking mode to measure transaction-set construction, validation, and application.
Changes:
- Adds queue-overfilling construction mode and timing metrics.
- Extends configuration, tests, and benchmark examples.
- Documents the new mode and metrics.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Review |
|---|---|
src/simulation/test/LoadGeneratorTests.cpp |
Adds SAC coverage; missing construction-mode coverage for CUSTOM_TOKEN and SOROSWAP (nit). |
src/simulation/ApplyLoad.h |
Declares construction timing helpers and state. |
src/simulation/ApplyLoad.cpp |
Implements construction timing, but duplicates and omits production nomination behavior and inflates global queue metrics (moderate). |
src/main/Config.h |
Adds the timing-mode enum value. |
src/main/Config.cpp |
Parses the new configuration value. |
src/herder/HerderImpl.h |
Declares the transaction-set build timer. |
src/herder/HerderImpl.cpp |
Instruments construction, but measures a mismatched span (moderate) and lacks performance evidence (nit). |
docs/software/commands.md |
Documents the mode as production nomination despite its synthetic implementation (moderate). |
docs/metrics.md |
Documents the build metric. |
docs/apply-load-ledger-limits.cfg |
Documents queue multiplier configuration. |
docs/apply-load-for-meta.cfg |
Documents queue multiplier configuration. |
docs/apply-load-benchmark-token.cfg |
Documents token benchmark configuration. |
docs/apply-load-benchmark-sac.cfg |
Documents SAC benchmark configuration. |
Suppressed comments (1)
src/herder/HerderImpl.cpp:1669
- The new production-path timer adds unconditional clock reads and metric updates to every leader nomination, but the PR provides no performance evidence (the checklist item is unchecked).
CONTRIBUTING.md:56-60requires evidence of improvement and no regression for performance-impacting changes; please include measurements that quantify this instrumentation and the new benchmark mode before merging.
auto const txSetBuildStart = std::chrono::steady_clock::now();
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Use production queue classes with a fresh backlog for each ledger. | ||
| // No aging or flooding is needed; admission warms the validation caches. | ||
| ClassicTransactionQueue classicQueue(mApp, 1, 1, txQueueMultiplier(false)); | ||
| SorobanTransactionQueue sorobanQueue(mApp, 1, 1, txQueueMultiplier(true), | ||
| {}); |
5adc49c to
d26b1a3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
src/herder/HerderImpl.cpp:1762
- The newly documented production
herder.txset.buildmetric has no test asserting thattriggerNextLedgerrecords a sample. The apply-load tests only assert the separate validation timer, so a misplaced start/end point or a missing update would go undetected. Add a Herder test that triggers nomination and verifies this timer's count increments exactly once.
mSCPMetrics.mTxSetBuild.Update(
std::chrono::duration_cast<std::chrono::nanoseconds>(
std::chrono::steady_clock::now() - txSetBuildStart));
| - `"txset-validation-and-apply"`: times tx-set construction, decoding, | ||
| validation and application through local consensus with a | ||
| single-validator quorum. Construction |
dmkozh
left a comment
There was a problem hiding this comment.
I've left some comments re measurements consistency between the apply modes, but I wonder if it's worth keeping two modes around in general. How much longer is a run of, say, 200/6000 SAC TPL ledgers with the txset stuff vs the old mode? If the difference is insignificant, then I don't see much point in maintaining different code paths instead of just the most comprehensive one.
| // catch some simple misconfiguration cases. | ||
| if (mMode == ApplyLoadMode::BENCHMARK_MODEL_TX) | ||
| if (measuresTxSetPhases() && | ||
| (txQueueMultiplier(true) < 2 || |
There was a problem hiding this comment.
I'm not sure we need this to be configurable. I'd rather reuse the actual constants that we are using for tx queue in all the modes. I don't see much value in benchmarking unrealistic scenarios, and supporting this config comes with quite a few lines of boilerplate like this check.
| { | ||
| if (!measuresTxSetPhases()) | ||
| { | ||
| return 1; |
There was a problem hiding this comment.
I'm not sure we should exclude this timing from the regular apply mode, it doesn't seem to cost us anything to add the txset building timing into the regular apply mode as well.
Moreover, I would argue that it might be somewhat misleading for the 'SCP' mode, as we should normally either nominate a tx set, or validate an external txset (I'm not actually 100% sure if that's the case now, but it probably should be). But for simplicity we can just always output it in all the scenarios, and use whichever number is more realistic if necessary.
| mApp.getLedgerManager().getLastClosedLedgerHeader().header.maxTxSetSize; | ||
| if (classicLimit > 0 && | ||
| (currentLimit < classicLimit || | ||
| (measuresTxSetPhases() && currentLimit != classicLimit))) |
There was a problem hiding this comment.
nit: I don't see much value in conditioning this on measurement method, we can always update the size exactly to the configured value.
| // processing, and externalization overhead. | ||
| otherShares.emplace_back(100.0 - validationShare - ledgerCloseShare); | ||
| } | ||
| if (!validationShares.empty()) |
There was a problem hiding this comment.
I don't fully understand the logging changes. I'm fine with whatever metrics that make sense, but just wanted to double-check that you're not dropping anything useful accidentally.
I agree for "benchmark" and "ledger-limits" modes, but not for `"max-sac-tps". For the modes where we just dump metrics on a set amount of load, I agree that they can be unified. If we still care about doing noisy binary search via "max-sac-tps", I don't think it makes much sense to only have the one mode. Or at least, we shouldn't count the aggregate time of construction, validation, and application towards our search of "max txs we can apply in the given window." We could just simply by having one mode, where all modes dump the stats of all phases, but for "max-sac-tps" we only count the apply phase timings towards our search, what do you think? Imo with features like parallel tx set download it doesn't make sense to time serial tx set build + validate steps towards any sort of max tps style of test. |
To be clear I meant just collapsing the measurements for the benchmark modes. I agree for max-sac-tps this doesn't make much sense, but arguably max-sac-tps doesn't make much sense anymore either (as we're using TPS target with time requirements as opposed to trying to maximize TPS within limited time, which makes much more sense for optimization anyways). |
Description
Adds a new mode to apply load, "txset-construction-and-apply". This mode times all leader work in a ledger, without any network work: tx set construction, validation, then application. It's the leader end-to-end variant of txset-validation-and-apply, which simulates a non-leader node with only tx set validation and application. In testing, it seems like the number of TXs in the queue has a significant impact on TX set building times, so unlike other apply load modes, this test will generate more TXs than will actually fit in the final block in order to overfill the queue.
Checklist
clang-formatv8.0.0 (viamake formator the Visual Studio extension)