Vivek1106-04 opened a new pull request, #12896: URL: https://github.com/apache/seatunnel/pull/12896
### Purpose of this pull request Related: #12418 (the benchmark this fixes), #12339 (STIP-40), #12165 (the change the benchmark is meant to evaluate). `CheckpointSchedulingBenchmark` scores keep falling through every measured iteration, most visibly at one pipeline, and the CV does not behave as expected across pipeline counts (reported by @nzw921rx on #12165). The full analysis is in https://github.com/apache/seatunnel/pull/12418#issuecomment-6135494803; in short: - The JIT compiles the trigger path by invocation count, and that path runs only 5 to 50 times a second. On GitHub's Linux runners the trigger body stays interpreted for about 80 s at one pipeline and reaches C2 only after 110 to 130 s at ten or more, while CI warms up for 30 s, so the measurement records the warmup curve on its way down. - At one pipeline the 200 ms interval floor gives the member 5 triggers a second instead of 50, so that point warms up ten times slower than the rest. This PR changes the benchmark code and its tests only: - Forks run with `-XX:CompileThresholdScaling=0.05`. (`0.01` compiles so much at once that more than 10% of triggers come due unmeasured and the skip limit fails the run.) - Each trial lets its own coordinators trigger 1,000 checkpoints, for at least 60 s, before JMH's warmup. Counting triggers rather than seconds gives the one-pipeline point the same compiled code as the others (about 200 s there, 60 s elsewhere). Warming up on throwaway coordinators does not work: cancelling them deoptimises `tryTriggerPendingCheckpoint` right before measurement. - The Javadoc documents the one-pipeline point as an idle-member point, to compare across revisions rather than against the other pipeline counts. #### Open choices for reviewers These were raised on #12418 and are still open; I can change either one in this PR: 1. **`-XX:CompileThresholdScaling=0.05` vs JVM defaults.** The alternative is to keep default thresholds and warm up for about 150 s per trial. That has not been run as a full sweep, and it leaves the one-pipeline body at a lower compilation tier than the other points. 2. **Cost.** The trigger-count warmup adds about 19 minutes per JDK to the `CheckpointSchedulingBenchmark` sweep in `benchmarks.yml`. Dropping the one-pipeline point (starting the sweep at ten pipelines) removes the noisiest point and about half of the added time. ### Does this PR introduce _any_ user-facing change? No. Benchmark module only. ### How was this patch tested? - `CheckpointSchedulingFixtureTest` and `CheckpointSchedulingBenchmarkTest` cover the trigger-count warmup, the minimum warmup time, and the fork JVM options. - Full sweep, `dev` against this change, with the exact command of `tools/benchmarks/run_benchmarks.sh` (3 forks, 3 warmup and 5 measurement iterations of 10 s). Both sides ran on the same runner, in both orders, on JDK 11 and 17, giving 12 forks per side and point: https://github.com/Vivek1106-04/seatunnel/actions/runs/38295747109 | pipelineNum | mean trend per iteration, dev | this PR | forks falling > 2% per iteration, dev | this PR | CV, dev | this PR | |---|---|---|---|---|---|---| | 1 | -3.5% | -0.3% | 8 / 12 | 5 / 12, scattered both ways | 10.5% | 9.4% | | 10 | -1.9% | +1.0% | 3 / 12 | 0 / 12 | 7.0% | 3.9% | | 100 | -1.5% | +0.3% | 3 / 12 | 0 / 12 | 4.5% | 3.6% | | 500 | -1.0% | -0.2% | 2 / 12 | 1 / 12 | 4.5% | 4.3% | No run of this change hit the skip limit. Per-iteration data for every fork is in the #12418 comment linked above. What remains is not a warmup effect: at one pipeline, iteration-to-iteration variation stays around 9 to 10% on both sides because the trigger's wake-up latency on an idle VM wanders by tens of microseconds, and forks still settle 1 to 5% apart. Reporting the per-iteration median instead of the mean would be a separate change to `save_jmh_result.py`. ### Check list * [x] If any new Jar binary package adding in your PR, please add License Notice according [New License Guide](https://github.com/apache/seatunnel/blob/dev/docs/en/developer/new-license.md) * [x] If necessary, please update the documentation to describe the new feature. https://github.com/apache/seatunnel/tree/dev/docs * [x] If necessary, please update `incompatible-changes.md` to describe the incompatibility caused by this PR. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
