You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by "arunpandianp (via GitHub)" <gi...@apache.org> on 2025/09/15 22:02:39 UTC
[PR] [Dataflow Streaming] Reuse proto builders on hot path to reduce GC overhead [beam]
arunpandianp opened a new pull request, #36164:
URL: https://github.com/apache/beam/pull/36164
#33578
Noticed it while debugging long GC on a high cardinality job.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Dataflow Streaming] Reuse proto builders on hot path to reduce GC overhead [beam]
Posted by "gemini-code-assist[bot] (via GitHub)" <gi...@apache.org>.
gemini-code-assist[bot] commented on code in PR #36164:
URL: https://github.com/apache/beam/pull/36164#discussion_r2350324027
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/windmill/client/grpc/GetWorkResponseChunkAssembler.java:
##########
@@ -94,15 +96,17 @@ List<AssembledWorkItem> append(Windmill.StreamingGetWorkResponseChunk chunk) {
*/
private Optional<AssembledWorkItem> flushToWorkItem() {
try {
+ workItemBuilder.mergeFrom(data);
return Optional.of(
AssembledWorkItem.create(
- WorkItem.parseFrom(data.newInput()),
+ workItemBuilder.build(),
Preconditions.checkNotNull(metadata),
workTimingInfosTracker.getLatencyAttributions(),
bufferedSize));
- } catch (IOException e) {
+ } catch (RuntimeException e) {
Review Comment:
The exception type in the `catch` block has been changed from `IOException` to `RuntimeException`. However, `workItemBuilder.mergeFrom(data)` (on line 99) can throw an `InvalidProtocolBufferException`, which is a checked exception that extends `IOException`. The `catch (RuntimeException e)` will not handle this checked exception, which will result in a compilation error since the `flushToWorkItem` method does not declare any thrown exceptions. Please revert the caught exception type to `IOException` to correctly handle parsing errors.
```suggestion
} catch (IOException e) {
```
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Dataflow Streaming] Reuse proto builders on hot path to reduce GC overhead [beam]
Posted by "arunpandianp (via GitHub)" <gi...@apache.org>.
arunpandianp commented on PR #36164:
URL: https://github.com/apache/beam/pull/36164#issuecomment-3294119212
R: @scwhittle
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Dataflow Streaming] Reuse proto builders on hot path to reduce GC overhead [beam]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36164:
URL: https://github.com/apache/beam/pull/36164#issuecomment-3294121576
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment `assign set of reviewers`
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Dataflow Streaming] Reuse proto builders on hot path to reduce GC overhead [beam]
Posted by "scwhittle (via GitHub)" <gi...@apache.org>.
scwhittle merged PR #36164:
URL: https://github.com/apache/beam/pull/36164
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Dataflow Streaming] Reuse proto builders on hot path to reduce GC overhead [beam]
Posted by "arunpandianp (via GitHub)" <gi...@apache.org>.
arunpandianp commented on code in PR #36164:
URL: https://github.com/apache/beam/pull/36164#discussion_r2350337177
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/windmill/client/grpc/GetWorkResponseChunkAssembler.java:
##########
@@ -94,15 +96,17 @@ List<AssembledWorkItem> append(Windmill.StreamingGetWorkResponseChunk chunk) {
*/
private Optional<AssembledWorkItem> flushToWorkItem() {
try {
+ workItemBuilder.mergeFrom(data);
return Optional.of(
AssembledWorkItem.create(
- WorkItem.parseFrom(data.newInput()),
+ workItemBuilder.build(),
Preconditions.checkNotNull(metadata),
workTimingInfosTracker.getLatencyAttributions(),
bufferedSize));
Review Comment:
Letting the unexpected runtimeException propagate up.
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Dataflow Streaming] Reuse proto builders on hot path to reduce GC overhead [beam]
Posted by "arunpandianp (via GitHub)" <gi...@apache.org>.
arunpandianp commented on PR #36164:
URL: https://github.com/apache/beam/pull/36164#issuecomment-3294323218
/gemini 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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Dataflow Streaming] Reuse proto builders on hot path to reduce GC overhead [beam]
Posted by "arunpandianp (via GitHub)" <gi...@apache.org>.
arunpandianp commented on PR #36164:
URL: https://github.com/apache/beam/pull/36164#issuecomment-3294288672
/gemini 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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Dataflow Streaming] Reuse proto builders on hot path to reduce GC overhead [beam]
Posted by "gemini-code-assist[bot] (via GitHub)" <gi...@apache.org>.
gemini-code-assist[bot] commented on code in PR #36164:
URL: https://github.com/apache/beam/pull/36164#discussion_r2350308956
##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/windmill/client/grpc/GetWorkResponseChunkAssembler.java:
##########
@@ -94,15 +96,17 @@ List<AssembledWorkItem> append(Windmill.StreamingGetWorkResponseChunk chunk) {
*/
private Optional<AssembledWorkItem> flushToWorkItem() {
try {
+ workItemBuilder.mergeFrom(data);
return Optional.of(
AssembledWorkItem.create(
- WorkItem.parseFrom(data.newInput()),
+ workItemBuilder.build(),
Preconditions.checkNotNull(metadata),
workTimingInfosTracker.getLatencyAttributions(),
bufferedSize));
Review Comment:
The change from `WorkItem.parseFrom(data.newInput())` to `workItemBuilder.build()` introduces a subtle but critical change in exception handling. `WorkItem.parseFrom()` throws an `InvalidProtocolBufferException` (which is a subclass of `IOException`) if a required field is missing, and this is caught by the existing `catch (IOException e)` block.
However, `workItemBuilder.build()` throws an `UninitializedMessageException`, which is a `RuntimeException` and is not caught. This could lead to an unhandled exception and crash the worker thread for data that would have been gracefully handled before.
To preserve the original error handling behavior, you should use `buildPartial()` and then check if the message is initialized, throwing an `InvalidProtocolBufferException` if it's not. This correctly mimics the behavior of `parseFrom()`.
```java
workItemBuilder.mergeFrom(data);
WorkItem workItem = workItemBuilder.buildPartial();
if (!workItem.isInitialized()) {
throw new org.apache.beam.vendor.grpc.v1p69p0.com.google.protobuf.InvalidProtocolBufferException(
"WorkItem not initialized").setUnfinishedMessage(workItem);
}
return Optional.of(
AssembledWorkItem.create(
workItem,
Preconditions.checkNotNull(metadata),
workTimingInfosTracker.getLatencyAttributions(),
bufferedSize));
```
--
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: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org