[GitHub] [kafka] showuon commented on a diff in pull request #13312: KAFKA-14766: Improve performance of VarInt encoding and decoding

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

showuon commented on code in PR #13312:
URL: https://github.com/apache/kafka/pull/13312#discussion_r1160405765


##########
clients/src/test/java/org/apache/kafka/common/utils/ByteUtilsTest.java:
##########
@@ -298,7 +414,7 @@ public void testReadInt() {
     }
 
     private void assertUnsignedVarintSerde(int value, byte[] expectedEncoding) throws IOException {
-        ByteBuffer buf = ByteBuffer.allocate(32);
+        ByteBuffer buf = ByteBuffer.allocate(MAX_LENGTH_VARINT);

Review Comment:
   Nice cleanup



##########
jmh-benchmarks/src/main/java/org/apache/kafka/jmh/util/ByteUtilsBenchmark.java:
##########
@@ -17,68 +17,241 @@
 
 package org.apache.kafka.jmh.util;
 
-import java.util.concurrent.ThreadLocalRandom;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.security.SecureRandom;
 import java.util.concurrent.TimeUnit;
 
 import org.apache.kafka.common.utils.ByteUtils;
 import org.openjdk.jmh.annotations.Benchmark;
+import org.openjdk.jmh.annotations.CompilerControl;
 import org.openjdk.jmh.annotations.Fork;
 import org.openjdk.jmh.annotations.Level;
 import org.openjdk.jmh.annotations.Measurement;
 import org.openjdk.jmh.annotations.OutputTimeUnit;
+import org.openjdk.jmh.annotations.Param;
 import org.openjdk.jmh.annotations.Scope;
 import org.openjdk.jmh.annotations.Setup;
 import org.openjdk.jmh.annotations.State;
 import org.openjdk.jmh.annotations.Warmup;
+import org.openjdk.jmh.infra.Blackhole;
 import org.openjdk.jmh.runner.Runner;
 import org.openjdk.jmh.runner.RunnerException;
 import org.openjdk.jmh.runner.options.Options;
 import org.openjdk.jmh.runner.options.OptionsBuilder;
 
-@State(Scope.Benchmark)
-@OutputTimeUnit(TimeUnit.MILLISECONDS)
+@OutputTimeUnit(TimeUnit.SECONDS)
 @Fork(3)
-@Warmup(iterations = 5, time = 1)
-@Measurement(iterations = 10, time = 1)
+@Warmup(iterations = 3, time = 1)
+@Measurement(iterations = 5, time = 1)
 public class ByteUtilsBenchmark {

Review Comment:
   Could we add some description for this `ByteUtilsBenchmark`, to explain what we tried to benchmark in this class, and which method we adopted in the end, and why... etc? Otherwise, other developers might get confused what we are trying to do in this test. 



##########
clients/src/main/java/org/apache/kafka/common/utils/ByteUtils.java:
##########
@@ -227,17 +268,73 @@ public static int readVarint(DataInput in) throws IOException {
      * @throws IOException              if {@link DataInput} throws {@link IOException}
      */
     public static long readVarlong(DataInput in) throws IOException {
-        long value = 0L;
-        int i = 0;
-        long b;
-        while (((b = in.readByte()) & 0x80) != 0) {
-            value |= (b & 0x7f) << i;
-            i += 7;
-            if (i > 63)
-                throw illegalVarlongException(value);
+        long raw = readUnsignedVarlong(in);
+        return (raw >>> 1) ^ -(raw & 1);
+    }
+
+    /**
+     * For implementation details see {@link #readUnsignedVarlong(ByteBuffer)}

Review Comment:
   I checked the javadoc in `#readUnsignedVarlong(ByteBuffer)`, it didn't explain any detailed implementation there. Could we copy the javadoc from `#readUnsignedVarlong(ByteBuffer)` to here? Or should we update the javadoc here?



-- 
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: jira-unsubscribe@kafka.apache.org

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