DanielLeens commented on PR #12173: URL: https://github.com/apache/seatunnel/pull/12173#issuecomment-6304984477
Thanks both, @nzw921rx for the same-worker run and @Rangsh for the careful breakdown. I agree with @nzw921rx's request to separate the production change from the benchmark change, and I read the code at `b137173f7` to see what that split would and would not tell us. **1. The `JobHistoryService` change cannot reach the row with the +510% CV.** The only benchmark caller of `storeFinishedPipelineMetrics` is `appendCompletedJobLifecycleBatch()` (`IMapJobGrowthBenchmarkWorkload.java:154`). `appendRunningJobBatch()` writes only the three running-job maps and never calls `JobHistoryService`. So `runningJobGrowth / initialStoredJobCount=1000` can only be affected by the fixture changes, not by the lock-and-merge change. The production change can only matter for `completedJobHistoryGrowth`, and that row moved +6.21% and +2.77% (favorable) with lower CV. Because of the confound in point 3, I would not read those as proof either, but the two changes are separable by row. **2. "The timed path is byte-identical" is true for the method bodies, but the keys they write are not.** In `prepareGrowthBatch()` the PR changes `GROWTH_KEY_BASE + index` into `GROWTH_KEY_BASE + growthBatchSequence++ * 100 + index`. On `dev` every iteration re-puts the same 100 keys that the previous iteration deleted. With this PR every iteration writes keys the map has never seen. That is set up off the clock, but it changes what the timed `put`s hit (fresh inserts versus re-inserts after a delete, and a different WAL key history). So the candidates for a difference are not only "between iterations": the per-iteration `loadAll` removal that @Rangsh already named, and the unique-key change, both touch the measured window indirectly. They are cheap to separate: - **A:** `dev` - **B:** `dev` plus only the benchmark files (`IMapJobStorageBenchmark`, `IMapJobGrowthBenchmarkWorkload`) - **C:** B plus `JobHistoryService` (the full PR) If B already shows the CV jump on `runningJobGrowth / 1000`, it is a fixture issue and the production change is out of the picture. If B is stable, C answers the remaining question for `completedJobHistoryGrowth`. If you want to go one level deeper, B1 (resident-only checks, original fixed keys) versus B2 (unique keys) splits point 2 from the `loadAll` removal. **3. The comparison builds the PR branch, not the PR merged into `dev`.** `benchmarks.yml` checks out `refs/pull/<n>/head` as the candidate and `dev` as the baseline, and each is built separately. This branch is currently 332 commits behind `dev`, so @Rangsh's confound point is correct: the table compares "merge-base engine plus fixture change" with "latest dev". One data point that supports "noise plus drift" (this is my inference, not a measurement): `taskGroupStateTransition / storedTaskGroupCount=1000` is not touched by this PR at all, yet it shows -12.05% with CV +50%. So rows at 1000-entry pressure on this worker already move by double digits without any change from this diff. **Suggested order:** sync the branch with `dev` first (merge or rebase; either is fine for the benchmark), then re-run A, B and C on the same worker, and compare the raw per-run numbers rather than only the medians. With two runs per side, a single outlier run is enough to produce a 32.89% CV. `-prof gc` on the `count=1000` row is a good follow-up if B still shows the jump, since it would confirm or rule out the GC-moved-into-the-window hypothesis. I cannot dispatch `Benchmarks` on `apache/seatunnel` myself, so that part needs @nzw921rx or another maintainer. Last, a quick closure on my earlier ask: the sampling-window Javadoc on `IMapJobGrowthBenchmarkWorkload` and the #12266 pointer are now in `b137173f7`, so that item is done from my side. The remaining gate is the benchmark question above plus the committer review. -- 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]
