pltbkd commented on PR #28305: URL: https://github.com/apache/flink/pull/28305#issuecomment-5084623193
> (I haven't done full review, will leave it for others) I generally think this change makes sense, but I'm not sure if this implementation will be efficient enough. Let's verify that via micro benchmarks. If there is no regression, then all is fine, otherwise we would have to think about a more efficient implementation. > > Please expand https://github.com/apache/flink-benchmarks/blob/master/src/main/java/org/apache/flink/benchmark/AsyncWaitOperatorBenchmark.java with a test case that would have `AsyncWaitOperator` followed by a network exchange (pointwise connection, doesn't have to be `keyBy`),with `DiscardingSink` in a separate task. > > And please run all of the benchmarks (with a focus on `AsyncWaitOperatorBenchmark`) and share the results to check if you haven't introduced any regression. Thanks for the suggestion! I've expanded `AsyncWaitOperatorBenchmark` with a `asyncWaitWithNetworkExchange` scenario using `disableChaining()` to force a network exchange (pointwise) with `DiscardingSink` in a separate task. I ran several times to cancel out correlated noise like GC/JIT jitter. Here's the result (positive = patch is faster): | Group | Scenarios | Avg Diff | Range (±SE) | |---|---|---|---| | With network exchange | ORDERED | +0.24% | ±3.9% | | With network exchange | UNORDERED | +4.50% | ±4.1% | | Without network exchange | ORDERED | -3.82% | ±3.1% | | Without network exchange | UNORDERED | -2.16% | ±1.8% | - When a downstream network exchange is present, there is **no regression** — the patch replaces the existing `recordWriter.isAvailable()` check with the chain-level availability provider, so no new overhead is introduced. - Without network exchange, there is a **small regression** (2–4%), because the patch adds a new check on a path that previously had none. However, this is consistent with the overhead introduced when the soft backpressure mechanism was originally added — this patch just extends its coverage to this scene. Do you find these results acceptable, or should we limit the scope so that without a downstream network exchange, we fall back to only checking `recordWriter` (always available) instead of applying the `SupportsSoftBackpressure` check? Full benchmark results (all 24 benchmark classes) are still running. My local environment has roughly ±3% noise, which makes it hard to distinguish small diffs. Is there a more stable way to run these benchmarks? -- 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]
