github-actions[bot] commented on code in PR #40951:
URL: https://github.com/apache/beam/pull/40951#discussion_r4446116383
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/SimpleParDoFn.java:
##########
@@ -209,14 +211,20 @@ public void abort() throws Exception {
helpers.abort();
}
+ /** Returns the {@link DoFnInfo} used by this {@link SimpleParDoFn}. */
+ @Override
+ public DoFnInfo<?, ?> getDoFnInfo() throws Exception {
+ return helpers.doFnInstanceManager.peek();
Review Comment:
**🟡 Medium**: When a bundle is active (`helpers.activeFnInfo != null`), the
`DoFnInfo` instance has been polled from `doFnInstanceManager` via `get()`. If
the pool queue is empty at that time, calling
`helpers.doFnInstanceManager.peek()` will deserialize a redundant `DoFnInfo`
copy and invoke `@Setup` on a second `DoFn` instance. Returning
`helpers.activeFnInfo` when non-null (matching the pattern in `processTimers()`
on lines 180-182, which could then also call `getDoFnInfo()`) avoids this extra
deserialization and `@Setup` call.
```suggestion
return helpers.activeFnInfo != null ? helpers.activeFnInfo :
helpers.doFnInstanceManager.peek();
```
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/StreamingKeyedWorkItemSideInputParDoFn.java:
##########
@@ -231,18 +233,25 @@ protected void
onProcessWindowedValue(WindowedValue<KeyedWorkItem<K, InputT>> el
}
if (hasState) {
helpers.registerStateCleanup(
- (WindowingStrategy<?, W>) getDoFnInfo().getWindowingStrategy(),
windowsProcessed);
+ (WindowingStrategy<?, W>)
helpers.activeFnInfo.getWindowingStrategy(), windowsProcessed);
}
}
+ /** Returns the {@link DoFnInfo} used by this {@link
StreamingKeyedWorkItemSideInputParDoFn}. */
+ @Override
+ public DoFnInfo<?, ?> getDoFnInfo() throws Exception {
+ return helpers.doFnInstanceManager.peek();
Review Comment:
**🟡 Medium**: Similar to `SimpleParDoFn.getDoFnInfo()`, prefer
`helpers.activeFnInfo` when a bundle is active so that calling `getDoFnInfo()`
mid-bundle does not trigger an unnecessary `DoFnInfo` deserialization and
`@Setup` invocation when the `DoFnInstanceManager` queue is empty.
```suggestion
return helpers.activeFnInfo != null ? helpers.activeFnInfo :
helpers.doFnInstanceManager.peek();
```
--
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]
