nzw921rx commented on PR #12173: URL: https://github.com/apache/seatunnel/pull/12173#issuecomment-6295072029
> Thanks for running the same-worker comparison, @nzw921rx — and yes, I see the `runningJobGrowth / initialStoredJobCount=1000` row: Score 270.871 → 381.309 us/op (+40.77% slower, since this is `Mode.SingleShotTime`/`Mode.AverageTime` — time per op, lower is better), CV 5.39% → 32.89%. Taking it seriously, not dismissing it as noise. > > Before reacting with code changes, three facts worth having on the record: > > 1. **The timed path is byte-identical to `dev`.** I diffed `appendRunningJobBatch()` / `appendCompletedJobLifecycleBatch()` between `dev@1ce2d78` and `b137173f7` — no difference. The regression can therefore only come from what happens _between_ iterations (non-timed setup/teardown), or from the build each side ran on, not from the measured code itself. > 2. **This comparison carries a large confound.** The workflow builds baseline and candidate separately (`./mvnw -Pbenchmark -pl seatunnel-benchmarks -am`), so the candidate ran against engine/storage code from the merge-base — currently 332 commits behind `dev`. The table measures "old engine + fixture change" vs "latest dev", so the fixture change and 332 commits of engine drift are not separable. > 3. **Sample size is thin for this scenario.** The PR comparison is `-f 1 -wi 3 -i 5` per run, 2 runs per side (B-C-C-B). For `initialStoredJobCount=1000` that's 10 measured iterations per revision against a 1000-job preloaded WAL. > > One concrete hypothesis I want to verify (not asserting it): on `dev`, the every-iteration `loadAll` replayed the full WAL off-clock, which also forced large off-clock allocations — GC effectively happened between iterations. Removing it may have moved GC pauses into the measured window, which would show up exactly as "slower + CV way up at 1000, CV down at 0" (the `count=0` row improved to 4.52%). A `-prof gc` run would confirm or kill this. > > Proposed next step, and I'll do whichever you prefer: > > * **(a)** Sync with latest `dev` first (you or another maintainer say the word — rebase or merge, either), then you re-dispatch the same-worker comparison on the synced head. That removes the 332-commit confound and gives an attributable result. > * **(b)** If you'd rather see diagnostics first, I can push a `-prof gc` fork run to show whether GC time moved into the measured window at `count=1000`. > > If after (a) the regression is still there, I agree it's a real fixture problem and I'll fix it in this PR (the likely lever is per-iteration key reuse or removing cross-iteration tombstone growth) rather than defer it. Thank you for your reply. First, separate the production code changes from the benchmark changes to verify whether the CV reduction is caused by production code changes or measurement issues -- 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]
