Re: [PR] [Dataflow Streaming] Reuse ByteStringOutputStream buffers to reduce GC overhead [beam]

[via GitHub]

gemini-code-assist[bot] commented on code in PR #36165:
URL: https://github.com/apache/beam/pull/36165#discussion_r2350328610


##########
runners/google-cloud-dataflow-java/worker/src/main/java/org/apache/beam/runners/dataflow/worker/windmill/state/WindmillStateUtil.java:
##########
@@ -26,22 +27,39 @@
 
 class WindmillStateUtil {
 
+  private static final ThreadLocal<SoftReference<ByteStringOutputStream>> 
threadLocalOutputStream =
+      new ThreadLocal<>();
+
   /** Encodes the given namespace and address as {@code 
&lt;namespace&gt;+&lt;address&gt;}. */
   @VisibleForTesting
   static ByteString encodeKey(StateNamespace namespace, StateTag<?> address) {
+    // Use ByteStringOutputStream rather than concatenation and String.format. 
We build these keys
+    // a lot, and this leads to better performance results. See associated 
benchmarks.
+    ByteStringOutputStream stream = getByteStringOutputStream();
     try {
-      // Use ByteStringOutputStream rather than concatenation and 
String.format. We build these keys
-      // a lot, and this leads to better performance results. See associated 
benchmarks.
-      ByteStringOutputStream stream = new ByteStringOutputStream();
       // stringKey starts and ends with a slash.  We separate it from the
       // StateTag ID by a '+' (which is guaranteed not to be in the stringKey) 
because the
       // ID comes from the user.
       namespace.appendTo(stream);
       stream.append('+');
       address.appendTo(stream);
-      return stream.toByteString();
+      return stream.toByteStringAndReset();
     } catch (IOException e) {
+      stream.toByteStringAndReset();
       throw new RuntimeException(e);
+    } catch (RuntimeException e) {
+      stream.toByteStringAndReset();
+      throw e;
+    }

Review Comment:
   ![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg)
   
   The two `catch` blocks contain duplicated code 
(`stream.toByteStringAndReset()`). You can combine them into a single `catch 
(Exception e)` block to reduce code duplication and improve maintainability. 
Inside the block, you can re-throw `RuntimeException`s directly and wrap other 
exceptions (like `IOException`) in a `RuntimeException`.
   
   ```suggestion
       } catch (Exception e) {
         stream.toByteStringAndReset();
         if (e instanceof RuntimeException) {
           throw (RuntimeException) e;
         }
         throw new RuntimeException(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-unsubscr...@beam.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org