Re: [PR] [CELEBORN-1073] Avoid memory copying in TransportMessage.toByteBuffer by using wrapping payload into CompositeByteBuf [incubator-celeborn]

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

waitinfuture commented on code in PR #2021:
URL: https://github.com/apache/incubator-celeborn/pull/2021#discussion_r1381087165


##########
common/src/main/java/org/apache/celeborn/common/network/protocol/TransportMessage.java:
##########
@@ -97,13 +99,10 @@ public <T extends GeneratedMessageV3> T getParsedPayload() throws InvalidProtoco
   }
 
   public ByteBuffer toByteBuffer() {
-    int totalBufferSize = payload.length + 4 + 4;
-    ByteBuffer buffer = ByteBuffer.allocate(totalBufferSize);
-    buffer.putInt(messageTypeValue);
-    buffer.putInt(payload.length);
-    buffer.put(payload);
-    buffer.flip();
-    return buffer;
+    ByteBuf headerBuf = Unpooled.buffer().writeInt(messageTypeValue).writeInt(payload.length);

Review Comment:
   `Unpooled.buffer()` allocates 256 bytes as default `static final int DEFAULT_INITIAL_CAPACITY = 256;`, which is much larger than the needed 8 bytes. Additionally, `toByteBuffer` is used for few messages whose payload is not big, IMO it's OK to leave what it is.



-- 
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: issues-unsubscribe@celeborn.apache.org

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