Do more speculative VarInt reading optimizations

Specifically, I tweaked `MinecraftVarintFrameDecoder` a little bit, and added some logic to unroll the first iteration of `ProtocolUtils.readVarInt()` if we can.

I haven't evaluated the performance of these changes.

proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java

@@ -104,6 +104,7 @@ public enum ProtocolUtils {
           .build();
 
   public static final int DEFAULT_MAX_STRING_SIZE = 65536; // 64KiB
+  private static final int MAXIMUM_VARINT_SIZE = 5;
   private static final BinaryTagType<? extends BinaryTag>[] BINARY_TAG_TYPES = new BinaryTagType[] {
       BinaryTagTypes.END, BinaryTagTypes.BYTE, BinaryTagTypes.SHORT, BinaryTagTypes.INT,
       BinaryTagTypes.LONG, BinaryTagTypes.FLOAT, BinaryTagTypes.DOUBLE,
@@ -127,49 +128,24 @@ public enum ProtocolUtils {
    * @return the decoded VarInt
    */
   public static int readVarInt(ByteBuf buf) {
-    int read = readVarIntSafely(buf);
-    if (read == Integer.MIN_VALUE) {
-      throw MinecraftDecoder.DEBUG ? new CorruptedFrameException("Bad VarInt decoded")
+    int readable = buf.readableBytes();
+    if (readable == 0) {
+      // special case for empty buffer
+      throw MinecraftDecoder.DEBUG ? new CorruptedFrameException("No bytes readable")
           : BAD_VARINT_CACHED;
     }
-    return read;
-  }
 
-  /**
-   * Reads a Minecraft-style VarInt from the specified {@code buf}. The difference between this
-   * method and {@link #readVarInt(ByteBuf)} is that this function returns a sentinel value if the
-   * varint is invalid.
-   *
-   * @param buf the buffer to read from
-   * @return the decoded VarInt, or {@code Integer.MIN_VALUE} if the varint is invalid
-   */
-  public static int readVarIntSafely(ByteBuf buf) {
-    int i = 0;
-    int maxRead = Math.min(5, buf.readableBytes());
-    for (int j = 0; j < maxRead; j++) {
-      int k = buf.readByte();
-      i |= (k & 0x7F) << j * 7;
-      if ((k & 0x80) != 128) {
-        return i;
-      }
+    // we can read at least one byte, and this should be a common case
+    int k = buf.readUnsignedByte();
+    if (k < 0x80) {
+      return k;
     }
-    return Integer.MIN_VALUE;
-  }
 
-  /**
-   * Reads a Minecraft-style VarInt from the specified {@code buf}. The difference between this
-   * method and {@link #readVarInt(ByteBuf)} is that this function returns a sentinel value if the
-   * varint is invalid.
-   *
-   * @param buf the buffer to read from
-   * @return the decoded VarInt
-   * @throws DecoderException if the varint is invalid
-   */
-  public static int readVarIntSafelyOrThrow(ByteBuf buf) {
-    int i = 0;
-    int maxRead = Math.min(5, buf.readableBytes());
-    for (int j = 0; j < maxRead; j++) {
-      int k = buf.readByte();
+    // in case decoding one byte was not enough, use a loop to decode up to the next 4 bytes
+    int maxRead = Math.min(MAXIMUM_VARINT_SIZE, readable);
+    int i = k & 0x7F;
+    for (int j = 1; j < maxRead; j++) {
+      k = buf.readByte();
       i |= (k & 0x7F) << j * 7;
       if ((k & 0x80) != 128) {
         return i;
@@ -210,6 +186,8 @@ public static void writeVarInt(ByteBuf buf, int value) {
 
   private static void writeVarIntFull(ByteBuf buf, int value) {
     // See https://steinborn.me/posts/performance/how-fast-can-you-write-a-varint/
+
+    // This essentially is an unrolled version of the "traditional" VarInt encoding.
     if ((value & (0xFFFFFFFF << 7)) == 0) {
       buf.writeByte(value);
     } else if ((value & (0xFFFFFFFF << 14)) == 0) {

proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftVarintFrameDecoder.java

@@ -17,7 +17,7 @@
 
 package com.velocitypowered.proxy.protocol.netty;
 
-import com.velocitypowered.proxy.protocol.netty.VarintByteDecoder.DecodeResult;
+import com.velocitypowered.proxy.protocol.netty.TwentyOneBitVarintByteDecoder.DecodeResult;
 import com.velocitypowered.proxy.util.except.QuietDecoderException;
 import io.netty.buffer.ByteBuf;
 import io.netty.channel.ChannelHandlerContext;
@@ -29,9 +29,9 @@
  */
 public class MinecraftVarintFrameDecoder extends ByteToMessageDecoder {
 
-  private static final QuietDecoderException BAD_LENGTH_CACHED =
+  private static final QuietDecoderException BAD_PACKET_LENGTH =
       new QuietDecoderException("Bad packet length");
-  private static final QuietDecoderException VARINT_BIG_CACHED =
+  private static final QuietDecoderException VARINT_TOO_BIG =
       new QuietDecoderException("VarInt too big");
 
   @Override
@@ -41,41 +41,50 @@ protected void decode(ChannelHandlerContext ctx, ByteBuf in, List<Object> out) {
       return;
     }
 
-    final VarintByteDecoder reader = new VarintByteDecoder();
+    final TwentyOneBitVarintByteDecoder reader = new TwentyOneBitVarintByteDecoder();
 
     int varintEnd = in.forEachByte(reader);
     if (varintEnd == -1) {
       // We tried to go beyond the end of the buffer. This is probably a good sign that the
       // buffer was too short to hold a proper varint.
       if (reader.getResult() == DecodeResult.RUN_OF_ZEROES) {
-        // Special case where the entire packet is just a run of zeroes. We ignore them all.
+        // If the packet is literally all zeroes, we can just ignore everything.
         in.clear();
       }
       return;
     }
 
-    if (reader.getResult() == DecodeResult.RUN_OF_ZEROES) {
-      // this will return to the point where the next varint starts
-      in.readerIndex(varintEnd);
-    } else if (reader.getResult() == DecodeResult.SUCCESS) {
-      int readVarint = reader.getReadVarint();
-      int bytesRead = reader.getBytesRead();
-      if (readVarint < 0) {
+    switch (reader.getResult()) {
+      case RUN_OF_ZEROES:
+        // We didn't decode anything useful, so we can just skip over the zeroes.
+        in.readerIndex(varintEnd);
+        break;
+      case TOO_SHORT:
+        // This case shouldn't happen (we check if we only have a partial varint above), but if it
+        // does, we just wait for more data.
+        break;
+      case TOO_BIG:
+        // Invalid varint, clear the buffer and close the connection (by throwing an exception).
         in.clear();
-        throw BAD_LENGTH_CACHED;
-      } else if (readVarint == 0) {
-        // skip over the empty packet(s) and ignore it
-        in.readerIndex(varintEnd + 1);
-      } else {
-        int minimumRead = bytesRead + readVarint;
-        if (in.isReadable(minimumRead)) {
-          out.add(in.retainedSlice(varintEnd + 1, readVarint));
-          in.skipBytes(minimumRead);
+        throw VARINT_TOO_BIG;
+      case SUCCESS:
+        // We decoded something. Do some sanity checks.
+        int len = reader.getReadVarint();
+        if (len < 0) {
+          // It's a negative length, which is invalid.
+          in.clear();
+          throw BAD_PACKET_LENGTH;
+        } else {
+          int varintLength = reader.getBytesRead();
+          if (in.isReadable(len + varintLength)) {
+            in.readerIndex(varintEnd + 1);
+            out.add(in.readRetainedSlice(len));
+          }
         }
-      }
-    } else if (reader.getResult() == DecodeResult.TOO_BIG) {
-      in.clear();
-      throw VARINT_BIG_CACHED;
+        break;
+      default:
+        // this should never happen
+        throw new AssertionError();
     }
   }
 }

proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/VarintByteDecoder.java → proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/TwentyOneBitVarintByteDecoder.java

@@ -19,22 +19,26 @@
 
 import io.netty.util.ByteProcessor;
 
-class VarintByteDecoder implements ByteProcessor {
+class TwentyOneBitVarintByteDecoder implements ByteProcessor {
 
   private int readVarint;
   private int bytesRead;
   private DecodeResult result = DecodeResult.TOO_SHORT;
 
   @Override
   public boolean process(byte k) {
+    // if we have a run of zeroes, we want to skip over them
     if (k == 0 && bytesRead == 0) {
-      // tentatively say it's invalid, but there's a possibility of redemption
       result = DecodeResult.RUN_OF_ZEROES;
       return true;
     }
     if (result == DecodeResult.RUN_OF_ZEROES) {
+      // if k is not zero, maybe we can decode a varint, but we don't track
+      // how many bytes we read, so break out now. `MinecraftVarintFrameDecoder`
+      // will skip over the zeroes and pick up where we left off.
       return false;
     }
+
     readVarint |= (k & 0x7F) << bytesRead++ * 7;
     if (bytesRead > 3) {
       result = DecodeResult.TOO_BIG;

proxy/src/main/java/com/velocitypowered/proxy/protocol/packet/LoginPluginMessagePacket.java

@@ -63,7 +63,7 @@ public String toString() {
 
   @Override
   public void decode(ByteBuf buf, ProtocolUtils.Direction direction, ProtocolVersion version) {
-    this.id = ProtocolUtils.readVarIntSafelyOrThrow(buf);
+    this.id = ProtocolUtils.readVarInt(buf);
     this.channel = ProtocolUtils.readString(buf);
     if (buf.isReadable()) {
       this.replace(buf.readRetainedSlice(buf.readableBytes()));

proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java

@@ -70,7 +70,7 @@ void testNegativeOld() {
   private void writeReadTestOld(ByteBuf buf, int test) {
     buf.clear();
     writeVarIntOld(buf, test);
-    assertEquals(test, ProtocolUtils.readVarIntSafely(buf));
+    assertEquals(test, ProtocolUtils.readVarInt(buf));
   }
 
   @Test
@@ -103,7 +103,7 @@ void testBytesWrittenAtBitBoundaries() {
           "Encoding of " + i + " was invalid");
 
       assertEquals(i, oldReadVarIntSafely(varintNew));
-      assertEquals(i, ProtocolUtils.readVarIntSafely(varintOld));
+      assertEquals(i, ProtocolUtils.readVarInt(varintOld));
 
       varintNew.clear();
       varintOld.clear();