[GitHub] [geode] dschneider-pivotal commented on a change in pull request #6943: GEODE-9641: Remove unnecessary allocation of HeapDataOutputStream

[GitBox <gi...@apache.org>]

dschneider-pivotal commented on a change in pull request #6943:
URL: https://github.com/apache/geode/pull/6943#discussion_r725195184



##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ReplyMessage.java
##########
@@ -274,10 +274,11 @@ public void toData(DataOutput out,
       SerializationContext context) throws IOException {
     super.toData(out, context);
 
-    final HeapDataOutputStream hdos =
-        new HeapDataOutputStream(context.getSerializationVersion());
+    HeapDataOutputStream hdos = null;
     boolean failedSerialization = false;
-    if (this.returnValueIsException || this.returnValue != null) {
+    final boolean hasReturnValue = this.returnValueIsException || this.returnValue != null;
+    if (hasReturnValue) {
+      hdos = new HeapDataOutputStream(context.getSerializationVersion());

Review comment:
       something else to consider in this method is should you even call hdos.close? If you look at it all it is doing is freeing up some heap resources and reseting the BufferDataOutputStream which actually mean you could continue to use it (which seems a bit off since close usually ends the life of an object). If you had a HDOS that was stored in something that was going to live longer than close could help free some memory up early. But in this case HDOS becomes garbage at the end of this method. The best practice in that case is to just let the garbage collector free it up and to not class close. But I'd be okay with you leaving the close call in but if it is going to cause a major refactor you might want to instead not call it.




-- 
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: notifications-unsubscribe@geode.apache.org

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