Skip to content

Improve binary search for apply-load.#5123

Open
dmkozh wants to merge 1 commit intostellar:masterfrom
dmkozh:apply_load_max_tps_fixes
Open

Improve binary search for apply-load.#5123
dmkozh wants to merge 1 commit intostellar:masterfrom
dmkozh:apply_load_max_tps_fixes

Conversation

@dmkozh
Copy link
Contributor

@dmkozh dmkozh commented Feb 4, 2026

Description

The binary search uses t-statistic to ensure the necessary confidence at each step. This results in needing less samples far away from the true value and more samples around it. We still need at least 30 samples though to keep math simple. This works reasonably well, but still doesn't avoid some fundamental variance issues that we have that result in very different and statistically significant performance difference between different runs. That concern should hopefully be addressed separately.

Also fix some SAC max TPS benchmark issues, the math got messed up after I've updated it from 1s granularity to 50ms.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

Copilot AI review requested due to automatic review settings February 4, 2026 02:34
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves the binary search algorithm used in apply-load benchmarks by implementing a statistically-driven approach using t-statistics. The changes ensure more reliable performance measurements by adapting sample sizes based on confidence levels, while requiring a minimum of 30 samples for statistical validity. Additionally, it fixes issues with SAC max TPS benchmarking where the math was incorrectly handling the granularity change from 1s to 50ms.

Changes:

  • Implemented new noisyBinarySearch function that uses t-statistics and adaptive sampling with configurable confidence levels
  • Refactored benchmark functions to return single-ledger timing measurements for use with the new binary search algorithm
  • Updated minimum ledger count requirement from >0 to ≥30 and removed the TARGET_CLOSE_TIME_STEP_MS validation constraint
  • Added comprehensive test coverage for the noisy binary search algorithm with multiple variance and function types

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
src/simulation/ApplyLoad.cpp Implements the new noisy binary search algorithm and refactors findMaxLimitsForModelTransaction and findMaxSacTps to use it
src/simulation/ApplyLoad.h Adds function declarations for the new benchmark functions and exports noisyBinarySearch for testing
src/simulation/test/LoadGeneratorTests.cpp Adds extensive test coverage for the noisy binary search with various scenarios and updates existing test configurations
src/main/Config.cpp Removes the validation requiring APPLY_LOAD_TARGET_CLOSE_TIME_MS to be a multiple of TARGET_CLOSE_TIME_STEP_MS
src/main/CommandLine.cpp Updates validation to require at least 30 ledgers and changes MIN_TPS comparison from >= to >
Builds/VisualStudio/stellar-core.vcxproj Updates Visual Studio project file to include previously missing source files
Builds/VisualStudio/stellar-core.vcxproj.filters Updates Visual Studio project filters to include previously missing source files

The binary search uses t-statistic to ensure the necessary confidence at each step. This results in needing less samples far away from the true value and more samples around it. We still need at least 30 samples though to keep math simple. This works reasonably well, but still doesn't avoid some fundamental variance issues that we have that result in very different and statistically significant performance difference between different runs. That concern should hopefully be addressed separately.

Also fix some SAC max TPS benchmark issues, the math got messed up after I've updated it from 1s granularity to 50ms.
@dmkozh dmkozh force-pushed the apply_load_max_tps_fixes branch from 18c5782 to cea4539 Compare February 4, 2026 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant