[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #805: [#804] improvement: Optimize CRC calculation of ByteBuffer

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

advancedxy commented on code in PR #805:
URL: https://github.com/apache/incubator-uniffle/pull/805#discussion_r1161383961


##########
common/src/main/java/org/apache/uniffle/common/util/ChecksumUtils.java:
##########
@@ -40,14 +40,19 @@ public static long getCrc32(byte[] buf, int offset, int length) {
     return crc32.getValue();
   }
 
-  // you may need to flip at first
   public static long getCrc32(ByteBuffer byteBuffer) {
-    if (byteBuffer.hasArray()) {
-      return getCrc32(byteBuffer.array(), byteBuffer.arrayOffset() + byteBuffer.position(), byteBuffer.remaining());
-    } else {
-      byte[] byteArray = new byte[byteBuffer.remaining()];
-      byteBuffer.get(byteArray);
-      return getCrc32(byteArray);
+    return getCrc32(byteBuffer, byteBuffer.position(), byteBuffer.limit() - byteBuffer.position());
+  }
+
+  public static long getCrc32(ByteBuffer byteBuffer, int offset, int length) {
+    CRC32 crc32 = new CRC32();
+    byteBuffer.position(offset);

Review Comment:
   You need to create a temp ByteBuffer via `byteBuffer.duplicate`  so that the position and limit is independent with the original byte buffer.
   
   The current method modifies the original byte buffer's position and limit, which is not expected.



-- 
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@uniffle.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org