[GitHub] [beam] scwhittle commented on a diff in pull request #25338: Change to a separate UnboundedScheduledExecutor for finalizations.

["scwhittle (via GitHub)" <gi...@apache.org>]

scwhittle commented on code in PR #25338:
URL: https://github.com/apache/beam/pull/25338#discussion_r1097743376


##########
sdks/java/harness/src/main/java/org/apache/beam/fn/harness/FnHarness.java:
##########
@@ -219,6 +219,10 @@ public static void main(
     ShortIdMap metricsShortIds = new ShortIdMap();
     ExecutorService executorService =
         options.as(ExecutorOptions.class).getScheduledExecutorService();
+    // In order to reduce memory spent on per-thread coders and buffers, we separate finalizations
+    // from standard bundle processing.
+    ExecutorService finalizationExecutorService =

Review Comment:
   Ah, I was confused by the default factory, but I guess that is just called once?
   If I didn't use the option but just used UnboundedScheduledExecutorService directly, what would break? ie why is it an option?
   
   If the thread pools are different, is there a benefit to having a single executor? We could share the task priority queue if we stored an additional executor with it.
   
   Another idea was to try to periodically figure out how much memory the thread was pinning and drop it got large.
   Even with separate groups, I'm concerned the per-thread objects could get big since they may just increase to the max observed.
   Would figuring that out for a thread periodically that be possible with jamm? While we're hoping, if we could discover thread groups dynamically based upon call-site that would be cool but probably difficult to do efficiently.



-- 
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