Vivek1106-04 commented on PR #12418: URL: https://github.com/apache/seatunnel/pull/12418#issuecomment-5799131107
@nzw921rx Good question. As I described it, the `@Benchmark` method would have waited for the trigger, so the score would have included most of the 10 s interval. The fix is to make the timed region start at the moment the trigger is due. JMH's own timer then measures `actual - expected`, and the metric is a normal JMH primary metric. ## Boundaries State (`Scope.Benchmark`): the real member, the P coordinators, and for each coordinator the time its last trigger was observed (`lastTriggerNanos`). `@Setup(Level.Invocation)`, not timed: 1. Pick the coordinator whose next trigger is due soonest, where `expected = lastTriggerNanos + interval`. The starts are spread across the interval, so at P = 500 with a 10 s interval one trigger is due every 20 ms. 2. Check that its previous checkpoint has completed (`pendingCounter == 0`). If it has not, this trigger would take the 500 ms pending re-arm, so skip this coordinator for this cycle and count the skip. 3. Park until about 1 ms before `expected`, then spin until `System.nanoTime() >= expected`. Parking alone would put the OS timer slack into the start of the timed region, so the last millisecond is a spin. 4. Check the trigger has not fired yet. It cannot fire before `expected` (see the note below), so this only catches a setup that overran. Count it as a skip too. `@Benchmark`, timed: spin on that coordinator's `pendingCounter` until it goes 0 -> 1, record the time in `lastTriggerNanos`, and return. So JMH's timed region runs from `expected` to "trigger observed". That is the scheduling delay plus the trigger's decision logic up to the executor submission, which is the boundary agreed earlier. Nothing is computed afterwards or returned for JMH to interpret: the delay is the invocation time. `@TearDown(Level.Iteration)` fails the run if skips go over a fixed share of invocations, so a run where most triggers took the re-arm path produces an error, not a number. Notes on precision: - `expected` is slightly early. `pendingCounter` is incremented just before the coordinator calls `schedule(..., interval)` for the next trigger, so the real deadline is a few microseconds after `lastTriggerNanos + interval`. Each sample therefore also includes the end of the previous trigger body. That code is the same on `dev` and #12165, so it adds a small, equal offset to both. - Both timestamps come from the same spin loop on the same thread, and the setup overhead JMH leaves outside the timed region is in the tens of nanoseconds. That is small against a delay measured in microseconds, which is why `Level.Invocation` is acceptable here even though it is usually not recommended. - If two coordinators are due within one sample, the second one's trigger happens while the first is being measured, so it has no fresh `lastTriggerNanos`. It is resynchronised on its next trigger and not sampled for that cycle. With 20 ms between due times this should be rare, and it is counted with the other skips. - The spinning thread uses one core for roughly 1 ms out of every 20 ms, which is about 5 % of one of the 4 CPUs. It is the same on both revisions. ## Reporting: SCORE and ERROR work unchanged, CV does not With `Mode.SampleTime`, SCORE is the mean invocation time and ERROR is JMH's confidence interval. `save_jmh_result.py` reads both from `primaryMetric` as it does for every other benchmark, and p50 / p99 / max are in `scorePercentiles` in the same JSON. CV is the gap. For sample modes JMH writes the raw data as `rawDataHistogram` (per fork, per iteration, `[value, count]` pairs), not `rawData`. `jmh_metrics()` only flattens `rawData`, so for any `SampleTime` benchmark it gets an empty list, `sample_standard_deviation` returns `0.0`, and the report shows CV = 0 %. No existing benchmark in `seatunnel-benchmarks` uses `SampleTime`, which is why this has not shown up. Two ways to handle it: - **(a) Keep `SampleTime` and teach `save_jmh_result.py` to read `rawDataHistogram`.** Compute each iteration's mean from its histogram and take the CV across those, so it means the same as the CV of the other modes (variation between iterations, not the spread of individual delays, which is expected to be wide). That is about ten lines plus a case in `test_save_jmh_result.py`. It changes shared tooling, so I would call it out in the PR description. - **(b) Use `Mode.AverageTime` with the same boundaries.** SCORE, ERROR and CV all work with no tool change, but the percentiles are lost, and for a scheduler the tail is the part that matters. I prefer (a). If you would rather keep the tooling out of this PR, I will use (b) here and do (a) in a follow-up. @DanielLeens Yes. The two reflected fields (`pendingCounter` and `CheckpointBarrierTriggerOperation.barrier`) and the two messages the fake task answers (barrier acknowledgement and `READY_START`) will each get a comment at the place they are read. The comment will say what engine behaviour it depends on and that the fixture fails at setup if the field is missing. -- 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]
