From ce26ae76c2cf93cb25be5356fdfb26f32820436b Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Thu, 22 Apr 2021 11:23:42 -0400 Subject: [PATCH 01/28] Fix case-sensitivity for ping passthrough --- .../proxy/connection/client/StatusSessionHandler.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/StatusSessionHandler.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/client/StatusSessionHandler.java index d17a419dc..792c5b458 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/client/StatusSessionHandler.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/connection/client/StatusSessionHandler.java @@ -41,6 +41,7 @@ import io.netty.buffer.ByteBuf; import java.net.InetSocketAddress; import java.util.ArrayList; import java.util.List; +import java.util.Locale; import java.util.Optional; import java.util.concurrent.CompletableFuture; import org.apache.logging.log4j.LogManager; @@ -167,6 +168,7 @@ public class StatusSessionHandler implements MinecraftSessionHandler { return CompletableFuture.completedFuture(constructLocalPing(shownVersion)); } else { String virtualHostStr = inbound.getVirtualHost().map(InetSocketAddress::getHostString) + .map(str -> str.toLowerCase(Locale.ROOT)) .orElse(""); List serversToTry = server.getConfiguration().getForcedHosts().getOrDefault( virtualHostStr, server.getConfiguration().getAttemptConnectionOrder()); From 92865b0307e052dcc19a2e958a37a768424cf875 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Thu, 22 Apr 2021 11:23:57 -0400 Subject: [PATCH 02/28] Velocity 1.1.5 --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index a1395e472..3b10333c4 100644 --- a/build.gradle +++ b/build.gradle @@ -17,7 +17,7 @@ allprojects { apply plugin: "com.github.spotbugs" group 'com.velocitypowered' - version '1.1.5-SNAPSHOT' + version '1.1.5' ext { // dependency versions From d7435fcbb67d299296bd98c3d99f6d19dc509fa1 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Thu, 22 Apr 2021 11:28:49 -0400 Subject: [PATCH 03/28] Velocity 1.1.6-SNAPSHOT --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index 3b10333c4..ac9db4ce6 100644 --- a/build.gradle +++ b/build.gradle @@ -17,7 +17,7 @@ allprojects { apply plugin: "com.github.spotbugs" group 'com.velocitypowered' - version '1.1.5' + version '1.1.6-SNAPSHOT' ext { // dependency versions From 81311e7516b826b695f1ba221653c96c221ae10b Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Sun, 25 Apr 2021 10:14:13 -0400 Subject: [PATCH 04/28] Don't attempt to decode empty buffers (fixes #482) Stuff like this makes me want to drop support for <=1.13 versions of Minecraft. --- .../velocitypowered/proxy/protocol/netty/MinecraftDecoder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftDecoder.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftDecoder.java index c648d3e16..5243a6796 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftDecoder.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftDecoder.java @@ -62,7 +62,7 @@ public class MinecraftDecoder extends ChannelInboundHandlerAdapter { } private void tryDecode(ChannelHandlerContext ctx, ByteBuf buf) throws Exception { - if (!ctx.channel().isActive()) { + if (!ctx.channel().isActive() || !buf.isReadable()) { buf.release(); return; } From 347853b94502abf9a7763d2b3029ce087f9514f4 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Sun, 25 Apr 2021 10:18:58 -0400 Subject: [PATCH 05/28] Disable hinting test for now --- .../com/velocitypowered/proxy/command/CommandManagerTests.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/proxy/src/test/java/com/velocitypowered/proxy/command/CommandManagerTests.java b/proxy/src/test/java/com/velocitypowered/proxy/command/CommandManagerTests.java index b9b40cc35..cdbee1759 100644 --- a/proxy/src/test/java/com/velocitypowered/proxy/command/CommandManagerTests.java +++ b/proxy/src/test/java/com/velocitypowered/proxy/command/CommandManagerTests.java @@ -46,6 +46,7 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import org.checkerframework.checker.nullness.qual.NonNull; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; public class CommandManagerTests { @@ -441,6 +442,8 @@ public class CommandManagerTests { .join().isEmpty()); } + // TODO: Hug needs to fix this test! + @Disabled @Test void testHinting() { VelocityCommandManager manager = createManager(); From 3db2fe8d63162c716b8ac57ae0898027ecd3a7bb Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 3 May 2021 17:31:32 -0400 Subject: [PATCH 06/28] Optimize varint writing Inspired by the approach described at the bottom of https://richardstartin.github.io/posts/dont-use-protobuf-for-telemetry Given that we do a lot of varint writing as well, this should provide a small performance boost for larger/complex packets whilst not regressing hard on smaller packets. --- .../proxy/protocol/ProtocolUtils.java | 26 ++++++++++++++----- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java index 50f6c48d0..f7424b05b 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java @@ -62,6 +62,17 @@ public enum ProtocolUtils { private static final int DEFAULT_MAX_STRING_SIZE = 65536; // 64KiB private static final QuietDecoderException BAD_VARINT_CACHED = new QuietDecoderException("Bad varint decoded"); + private static final int[] VAR_INT_LENGTHS = new int[33]; + + static { + // Inspired by https://richardstartin.github.io/posts/dont-use-protobuf-for-telemetry + // + // This has been slightly modified in that we reduce the length to 32-bit only, since Velocity + // doesn't look at any part of the Minecraft protocol that requires us to look at VarLongs. + for (int i = 0; i <= 32; ++i) { + VAR_INT_LENGTHS[i] = (31 - i) / 7; + } + } /** * Reads a Minecraft-style VarInt from the specified {@code buf}. @@ -97,21 +108,22 @@ public enum ProtocolUtils { return Integer.MIN_VALUE; } + public static int varintBytes(int value) { + return VAR_INT_LENGTHS[Integer.numberOfLeadingZeros(value)]; + } + /** * Writes a Minecraft-style VarInt to the specified {@code buf}. * @param buf the buffer to read from * @param value the integer to write */ public static void writeVarInt(ByteBuf buf, int value) { - while (true) { - if ((value & 0xFFFFFF80) == 0) { - buf.writeByte(value); - return; - } - - buf.writeByte(value & 0x7F | 0x80); + int length = varintBytes(value); + for (int i = 0; i < length; ++i) { + buf.writeByte(((byte) ((value & 0x7F) | 0x80))); value >>>= 7; } + buf.writeByte((byte) value); } public static String readString(ByteBuf buf) { From e531cdb373c654748f3c819b80a5286d9d431d21 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 3 May 2021 18:07:25 -0400 Subject: [PATCH 07/28] Revert "Optimize varint writing" This reverts commit 3db2fe8d63162c716b8ac57ae0898027ecd3a7bb. --- .../proxy/protocol/ProtocolUtils.java | 26 +++++-------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java index f7424b05b..50f6c48d0 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java @@ -62,17 +62,6 @@ public enum ProtocolUtils { private static final int DEFAULT_MAX_STRING_SIZE = 65536; // 64KiB private static final QuietDecoderException BAD_VARINT_CACHED = new QuietDecoderException("Bad varint decoded"); - private static final int[] VAR_INT_LENGTHS = new int[33]; - - static { - // Inspired by https://richardstartin.github.io/posts/dont-use-protobuf-for-telemetry - // - // This has been slightly modified in that we reduce the length to 32-bit only, since Velocity - // doesn't look at any part of the Minecraft protocol that requires us to look at VarLongs. - for (int i = 0; i <= 32; ++i) { - VAR_INT_LENGTHS[i] = (31 - i) / 7; - } - } /** * Reads a Minecraft-style VarInt from the specified {@code buf}. @@ -108,22 +97,21 @@ public enum ProtocolUtils { return Integer.MIN_VALUE; } - public static int varintBytes(int value) { - return VAR_INT_LENGTHS[Integer.numberOfLeadingZeros(value)]; - } - /** * Writes a Minecraft-style VarInt to the specified {@code buf}. * @param buf the buffer to read from * @param value the integer to write */ public static void writeVarInt(ByteBuf buf, int value) { - int length = varintBytes(value); - for (int i = 0; i < length; ++i) { - buf.writeByte(((byte) ((value & 0x7F) | 0x80))); + while (true) { + if ((value & 0xFFFFFF80) == 0) { + buf.writeByte(value); + return; + } + + buf.writeByte(value & 0x7F | 0x80); value >>>= 7; } - buf.writeByte((byte) value); } public static String readString(ByteBuf buf) { From 4ca97a6df9833ff8b402095698fc9746ab86c1b9 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 3 May 2021 19:14:48 -0400 Subject: [PATCH 08/28] Reapply "Optimize varint writing" Inspired by the approach described at the bottom of https://richardstartin.github.io/posts/dont-use-protobuf-for-telemetry Given that we do a lot of varint writing as well, this should provide a small performance boost for larger/complex packets whilst not regressing hard on smaller packets. This includes a test to ensure that the behavior is as expected and fixes the initialization loop so that the correct results will be given. Much thanks to @octylFractal for acting as my duck while trying to figure this out. --- .../proxy/protocol/ProtocolUtils.java | 26 ++++++++---- .../proxy/protocol/ProtocolUtilsTest.java | 42 +++++++++++++++++++ 2 files changed, 61 insertions(+), 7 deletions(-) create mode 100644 proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java index 50f6c48d0..6fbecf2bc 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java @@ -62,6 +62,17 @@ public enum ProtocolUtils { private static final int DEFAULT_MAX_STRING_SIZE = 65536; // 64KiB private static final QuietDecoderException BAD_VARINT_CACHED = new QuietDecoderException("Bad varint decoded"); + private static final int[] VAR_INT_BYTE_LENGTHS = new int[33]; + + static { + // Inspired by https://richardstartin.github.io/posts/dont-use-protobuf-for-telemetry + // This has been slightly modified in that we reduce the length to 32-bit only, since Velocity + // doesn't look at any part of the Minecraft protocol that requires us to look at VarLongs. + for (int i = 0; i <= 32; ++i) { + VAR_INT_BYTE_LENGTHS[i] = (int) Math.ceil((31d - (i - 1)) / 7d); + } + VAR_INT_BYTE_LENGTHS[32] = 1; // Special case for the number 0. + } /** * Reads a Minecraft-style VarInt from the specified {@code buf}. @@ -97,21 +108,22 @@ public enum ProtocolUtils { return Integer.MIN_VALUE; } + public static int varintBytes(int value) { + return VAR_INT_BYTE_LENGTHS[Integer.numberOfLeadingZeros(value)]; + } + /** * Writes a Minecraft-style VarInt to the specified {@code buf}. * @param buf the buffer to read from * @param value the integer to write */ public static void writeVarInt(ByteBuf buf, int value) { - while (true) { - if ((value & 0xFFFFFF80) == 0) { - buf.writeByte(value); - return; - } - - buf.writeByte(value & 0x7F | 0x80); + int length = varintBytes(value); + for (int i = 0; i < length; ++i) { + buf.writeByte(((byte) ((value & 0x7F) | 0x80))); value >>>= 7; } + buf.writeByte((byte) value); } public static String readString(ByteBuf buf) { diff --git a/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java b/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java new file mode 100644 index 000000000..35431d81a --- /dev/null +++ b/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java @@ -0,0 +1,42 @@ +package com.velocitypowered.proxy.protocol; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +import org.junit.jupiter.api.Test; + +public class ProtocolUtilsTest { + + @Test + void negativeVarIntBytes() { + assertEquals(5, ProtocolUtils.varintBytes(-1)); + assertEquals(5, ProtocolUtils.varintBytes(Integer.MIN_VALUE)); + } + + @Test + void zeroVarIntBytes() { + assertEquals(1, ProtocolUtils.varintBytes(0)); + assertEquals(1, ProtocolUtils.varintBytes(1)); + } + + @Test + void ensureConsistencyAcrossNumberBits() { + for (int i = 0; i <= 31; i++) { + int number = (1 << i) - 1; + assertEquals(conventionalWrittenBytes(number), ProtocolUtils.varintBytes(number), + "mismatch with " + i + "-bit number"); + } + } + + private int conventionalWrittenBytes(int value) { + int wouldBeWritten = 0; + while (true) { + if ((value & ~0x7FL) == 0) { + wouldBeWritten++; + return wouldBeWritten; + } else { + wouldBeWritten++; + value >>>= 7; + } + } + } +} From 0811ebb312267525fc5b6ff0c7e9ef0d3302df11 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 3 May 2021 19:18:06 -0400 Subject: [PATCH 09/28] Gotta add the license header to the test file --- .../proxy/protocol/ProtocolUtilsTest.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java b/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java index 35431d81a..d48be3742 100644 --- a/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java +++ b/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java @@ -1,3 +1,20 @@ +/* + * Copyright (C) 2018 Velocity Contributors + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + package com.velocitypowered.proxy.protocol; import static org.junit.jupiter.api.Assertions.assertEquals; From 0debb81392a70f33781756198133414eed5ba7f1 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 3 May 2021 20:08:16 -0400 Subject: [PATCH 10/28] I was nagged to make it more consistent --- .../proxy/protocol/ProtocolUtils.java | 16 ++++++++-------- .../proxy/protocol/ProtocolUtilsTest.java | 10 +++++----- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java index 6fbecf2bc..2591da8cc 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java @@ -61,17 +61,17 @@ public enum ProtocolUtils { private static final int DEFAULT_MAX_STRING_SIZE = 65536; // 64KiB private static final QuietDecoderException BAD_VARINT_CACHED = - new QuietDecoderException("Bad varint decoded"); - private static final int[] VAR_INT_BYTE_LENGTHS = new int[33]; + new QuietDecoderException("Bad VarInt decoded"); + private static final int[] VARINT_BYTE_LENGTHS = new int[33]; static { // Inspired by https://richardstartin.github.io/posts/dont-use-protobuf-for-telemetry // This has been slightly modified in that we reduce the length to 32-bit only, since Velocity // doesn't look at any part of the Minecraft protocol that requires us to look at VarLongs. for (int i = 0; i <= 32; ++i) { - VAR_INT_BYTE_LENGTHS[i] = (int) Math.ceil((31d - (i - 1)) / 7d); + VARINT_BYTE_LENGTHS[i] = (int) Math.ceil((31d - (i - 1)) / 7d); } - VAR_INT_BYTE_LENGTHS[32] = 1; // Special case for the number 0. + VARINT_BYTE_LENGTHS[32] = 1; // Special case for the number 0. } /** @@ -82,7 +82,7 @@ public enum ProtocolUtils { public static int readVarInt(ByteBuf buf) { int read = readVarIntSafely(buf); if (read == Integer.MIN_VALUE) { - throw MinecraftDecoder.DEBUG ? new CorruptedFrameException("Bad varint decoded") + throw MinecraftDecoder.DEBUG ? new CorruptedFrameException("Bad VarInt decoded") : BAD_VARINT_CACHED; } return read; @@ -108,8 +108,8 @@ public enum ProtocolUtils { return Integer.MIN_VALUE; } - public static int varintBytes(int value) { - return VAR_INT_BYTE_LENGTHS[Integer.numberOfLeadingZeros(value)]; + public static int varIntBytes(int value) { + return VARINT_BYTE_LENGTHS[Integer.numberOfLeadingZeros(value)]; } /** @@ -118,7 +118,7 @@ public enum ProtocolUtils { * @param value the integer to write */ public static void writeVarInt(ByteBuf buf, int value) { - int length = varintBytes(value); + int length = varIntBytes(value); for (int i = 0; i < length; ++i) { buf.writeByte(((byte) ((value & 0x7F) | 0x80))); value >>>= 7; diff --git a/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java b/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java index d48be3742..40800935d 100644 --- a/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java +++ b/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java @@ -25,21 +25,21 @@ public class ProtocolUtilsTest { @Test void negativeVarIntBytes() { - assertEquals(5, ProtocolUtils.varintBytes(-1)); - assertEquals(5, ProtocolUtils.varintBytes(Integer.MIN_VALUE)); + assertEquals(5, ProtocolUtils.varIntBytes(-1)); + assertEquals(5, ProtocolUtils.varIntBytes(Integer.MIN_VALUE)); } @Test void zeroVarIntBytes() { - assertEquals(1, ProtocolUtils.varintBytes(0)); - assertEquals(1, ProtocolUtils.varintBytes(1)); + assertEquals(1, ProtocolUtils.varIntBytes(0)); + assertEquals(1, ProtocolUtils.varIntBytes(1)); } @Test void ensureConsistencyAcrossNumberBits() { for (int i = 0; i <= 31; i++) { int number = (1 << i) - 1; - assertEquals(conventionalWrittenBytes(number), ProtocolUtils.varintBytes(number), + assertEquals(conventionalWrittenBytes(number), ProtocolUtils.varIntBytes(number), "mismatch with " + i + "-bit number"); } } From c041bea1b6ff82469313094c312f84a9c39e5ffa Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Mon, 3 May 2021 20:17:51 -0400 Subject: [PATCH 11/28] Avoid calling writeVarInt in the (very) common uncompressed packet case --- .../proxy/protocol/netty/MinecraftCompressEncoder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressEncoder.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressEncoder.java index f6cb495ca..356c27231 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressEncoder.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressEncoder.java @@ -39,7 +39,7 @@ public class MinecraftCompressEncoder extends MessageToByteEncoder { int uncompressed = msg.readableBytes(); if (uncompressed < threshold) { // Under the threshold, there is nothing to do. - ProtocolUtils.writeVarInt(out, 0); + out.writeByte(0); out.writeBytes(msg); } else { ProtocolUtils.writeVarInt(out, uncompressed); From 2713831f77722eef4c34160515649ad3dc3cb9da Mon Sep 17 00:00:00 2001 From: Riley Park Date: Mon, 3 May 2021 19:38:39 -0700 Subject: [PATCH 12/28] Revert "Avoid calling writeVarInt in the (very) common uncompressed packet case" This reverts commit c041bea1b6ff82469313094c312f84a9c39e5ffa. --- .../proxy/protocol/netty/MinecraftCompressEncoder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressEncoder.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressEncoder.java index 356c27231..f6cb495ca 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressEncoder.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressEncoder.java @@ -39,7 +39,7 @@ public class MinecraftCompressEncoder extends MessageToByteEncoder { int uncompressed = msg.readableBytes(); if (uncompressed < threshold) { // Under the threshold, there is nothing to do. - out.writeByte(0); + ProtocolUtils.writeVarInt(out, 0); out.writeBytes(msg); } else { ProtocolUtils.writeVarInt(out, uncompressed); From 3dc8e25ec7e9c8f3dac71f0c825dc7cd20fc6ee0 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Tue, 4 May 2021 16:30:32 -0400 Subject: [PATCH 13/28] Fix varint writing (for real!) Thanks to @Leymooo for reporting and providing test cases. (I also added one of my own tests which proved the most useful to debugging the issue.) --- .../proxy/protocol/ProtocolUtils.java | 23 +++-- .../proxy/protocol/ProtocolUtilsTest.java | 94 +++++++++++++++++++ 2 files changed, 108 insertions(+), 9 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java index 2591da8cc..9ac62ff69 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java @@ -62,16 +62,13 @@ public enum ProtocolUtils { private static final int DEFAULT_MAX_STRING_SIZE = 65536; // 64KiB private static final QuietDecoderException BAD_VARINT_CACHED = new QuietDecoderException("Bad VarInt decoded"); - private static final int[] VARINT_BYTE_LENGTHS = new int[33]; + private static final int[] VARINT_EXACT_BYTE_LENGTHS = new int[33]; static { - // Inspired by https://richardstartin.github.io/posts/dont-use-protobuf-for-telemetry - // This has been slightly modified in that we reduce the length to 32-bit only, since Velocity - // doesn't look at any part of the Minecraft protocol that requires us to look at VarLongs. for (int i = 0; i <= 32; ++i) { - VARINT_BYTE_LENGTHS[i] = (int) Math.ceil((31d - (i - 1)) / 7d); + VARINT_EXACT_BYTE_LENGTHS[i] = (int) Math.ceil((31d - (i - 1)) / 7d); } - VARINT_BYTE_LENGTHS[32] = 1; // Special case for the number 0. + VARINT_EXACT_BYTE_LENGTHS[32] = 1; // Special case for the number 0. } /** @@ -108,8 +105,13 @@ public enum ProtocolUtils { return Integer.MIN_VALUE; } + /** + * Returns the exact byte size of {@code value} if it were encoded as a VarInt. + * @param value the value to encode + * @return the byte size of {@code value} if encoded as a VarInt + */ public static int varIntBytes(int value) { - return VARINT_BYTE_LENGTHS[Integer.numberOfLeadingZeros(value)]; + return VARINT_EXACT_BYTE_LENGTHS[Integer.numberOfLeadingZeros(value)]; } /** @@ -118,8 +120,11 @@ public enum ProtocolUtils { * @param value the integer to write */ public static void writeVarInt(ByteBuf buf, int value) { - int length = varIntBytes(value); - for (int i = 0; i < length; ++i) { + // Inspired by https://richardstartin.github.io/posts/dont-use-protobuf-for-telemetry + // This has been slightly modified in that we reduce the length to 32-bit only, since Velocity + // doesn't look at any part of the Minecraft protocol that requires us to look at VarLongs. + int continuationBytes = (31 - Integer.numberOfLeadingZeros(value)) / 7; + for (int i = 0; i < continuationBytes; ++i) { buf.writeByte(((byte) ((value & 0x7F) | 0x80))); value >>>= 7; } diff --git a/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java b/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java index 40800935d..43b940806 100644 --- a/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java +++ b/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java @@ -17,8 +17,13 @@ package com.velocitypowered.proxy.protocol; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import io.netty.buffer.ByteBuf; +import io.netty.buffer.ByteBufUtil; +import io.netty.buffer.Unpooled; import org.junit.jupiter.api.Test; public class ProtocolUtilsTest { @@ -44,6 +49,95 @@ public class ProtocolUtilsTest { } } + @Test + void testPositiveOld() { + ByteBuf buf = Unpooled.buffer(5); + for (int i = 0; i >= 0; i += 127) { + writeReadTestOld(buf, i); + } + } + + @Test + void testNegativeOld() { + ByteBuf buf = Unpooled.buffer(5); + for (int i = 0; i <= 0; i -= 127) { + writeReadTestOld(buf, i); + } + } + + private void writeReadTestOld(ByteBuf buf, int test) { + buf.clear(); + writeVarIntOld(buf, test); + assertEquals(test, ProtocolUtils.readVarIntSafely(buf)); + } + + @Test + void testBytesWrittenAtBitBoundaries() { + ByteBuf varintNew = Unpooled.buffer(5); + ByteBuf varintOld = Unpooled.buffer(5); + + long bytesNew = 0; + long bytesOld = 0; + for (int bit = 0; bit <= 31; bit++) { + int i = (1 << bit) - 1; + + writeVarIntOld(varintOld, i); + ProtocolUtils.writeVarInt(varintNew, i); + assertArrayEquals(varintOld.array(), varintNew.array(), + "Encoding of " + i + " was invalid"); + + assertEquals(i, oldReadVarIntSafely(varintNew)); + assertEquals(i, ProtocolUtils.readVarIntSafely(varintOld)); + + varintNew.clear(); + varintOld.clear(); + } + assertEquals(bytesNew, bytesOld, "byte sizes differ"); + } + + @Test + void testBytesWritten() { + ByteBuf varintNew = Unpooled.buffer(5); + ByteBuf varintOld = Unpooled.buffer(5); + + long bytesNew = 0; + long bytesOld = 0; + for (int i = 0; i <= 1_000_000; i++) { + ProtocolUtils.writeVarInt(varintNew, i); + writeVarIntOld(varintOld, i); + bytesNew += varintNew.readableBytes(); + bytesOld += varintOld.readableBytes(); + varintNew.clear(); + varintOld.clear(); + } + assertEquals(bytesNew, bytesOld, "byte sizes differ"); + } + + private static int oldReadVarIntSafely(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; + } + } + return Integer.MIN_VALUE; + } + + private void writeVarIntOld(ByteBuf buf, int value) { + while (true) { + if ((value & 0xFFFFFF80) == 0) { + buf.writeByte(value); + return; + } + + buf.writeByte(value & 0x7F | 0x80); + value >>>= 7; + } + } + private int conventionalWrittenBytes(int value) { int wouldBeWritten = 0; while (true) { From 7c76ae9a272cf90796429cf0141ccea042c7f6ac Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Tue, 4 May 2021 16:32:37 -0400 Subject: [PATCH 14/28] Make sure to check not only the entire array but also how much was written --- .../com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java b/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java index 43b940806..57cd8f7da 100644 --- a/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java +++ b/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java @@ -83,7 +83,7 @@ public class ProtocolUtilsTest { writeVarIntOld(varintOld, i); ProtocolUtils.writeVarInt(varintNew, i); - assertArrayEquals(varintOld.array(), varintNew.array(), + assertArrayEquals(ByteBufUtil.getBytes(varintOld), ByteBufUtil.getBytes(varintNew), "Encoding of " + i + " was invalid"); assertEquals(i, oldReadVarIntSafely(varintNew)); From e0153267db1174c35f6d20be7f6cf8eea27ec097 Mon Sep 17 00:00:00 2001 From: Nicolas RAYNAUD Date: Wed, 5 May 2021 00:22:50 +0200 Subject: [PATCH 15/28] Fixed copyright year (#490) --- .../velocitypowered/proxy/command/builtin/VelocityCommand.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/command/builtin/VelocityCommand.java b/proxy/src/main/java/com/velocitypowered/proxy/command/builtin/VelocityCommand.java index f4b98881b..6eaf7d69e 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/command/builtin/VelocityCommand.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/command/builtin/VelocityCommand.java @@ -228,7 +228,7 @@ public class VelocityCommand implements SimpleCommand { .append(Component.text(version.getVersion()).decoration(TextDecoration.BOLD, false)) .build(); TextComponent copyright = Component - .text("Copyright 2018-2020 " + version.getVendor() + ". " + version.getName() + .text("Copyright 2018-2021 " + version.getVendor() + ". " + version.getName() + " is licensed under the terms of the GNU General Public License v3."); source.sendMessage(Identity.nil(), velocity); source.sendMessage(Identity.nil(), copyright); From d42cc4f984656e5ff394ed4c22bfebc5eeaf73eb Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Tue, 4 May 2021 18:51:01 -0400 Subject: [PATCH 16/28] Force a flush after a certain threshold of packets have been queued for sending Fixes #486. --- .../backend/BackendPlaySessionHandler.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendPlaySessionHandler.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendPlaySessionHandler.java index d649b4c07..b3c7133cf 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendPlaySessionHandler.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/connection/backend/BackendPlaySessionHandler.java @@ -56,12 +56,16 @@ public class BackendPlaySessionHandler implements MinecraftSessionHandler { private static final Logger logger = LogManager.getLogger(BackendPlaySessionHandler.class); private static final boolean BACKPRESSURE_LOG = Boolean .getBoolean("velocity.log-server-backpressure"); + private static final int MAXIMUM_PACKETS_TO_FLUSH = Integer + .getInteger("velocity.max-packets-per-flush", 8192); + private final VelocityServer server; private final VelocityServerConnection serverConn; private final ClientPlaySessionHandler playerSessionHandler; private final MinecraftConnection playerConnection; private final BungeeCordMessageResponder bungeecordMessageResponder; private boolean exceptionTriggered = false; + private int packetsFlushed; BackendPlaySessionHandler(VelocityServer server, VelocityServerConnection serverConn) { this.server = server; @@ -271,16 +275,25 @@ public class BackendPlaySessionHandler implements MinecraftSessionHandler { ((PluginMessage) packet).retain(); } playerConnection.delayedWrite(packet); + if (++packetsFlushed >= MAXIMUM_PACKETS_TO_FLUSH) { + playerConnection.flush(); + packetsFlushed = 0; + } } @Override public void handleUnknown(ByteBuf buf) { playerConnection.delayedWrite(buf.retain()); + if (++packetsFlushed >= MAXIMUM_PACKETS_TO_FLUSH) { + playerConnection.flush(); + packetsFlushed = 0; + } } @Override public void readCompleted() { playerConnection.flush(); + packetsFlushed = 0; } @Override From 1cef82d54dda8726dcb5708b69fa51b04c215439 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Wed, 5 May 2021 22:13:54 -0400 Subject: [PATCH 17/28] Unroll the VarInt writing loop This is about as optimized as it can get. Thanks to @Leymooo for the idea, I simply expanded on it. We optimize for the common 1-3 byte cases, and punt more "complicated" cases to the original VarInt writing function we had before. --- .../proxy/protocol/ProtocolUtils.java | 31 ++++++++++++++----- .../netty/MinecraftVarintLengthEncoder.java | 3 +- 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java index 9ac62ff69..1f696bb7a 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java @@ -120,15 +120,32 @@ public enum ProtocolUtils { * @param value the integer to write */ public static void writeVarInt(ByteBuf buf, int value) { - // Inspired by https://richardstartin.github.io/posts/dont-use-protobuf-for-telemetry - // This has been slightly modified in that we reduce the length to 32-bit only, since Velocity - // doesn't look at any part of the Minecraft protocol that requires us to look at VarLongs. - int continuationBytes = (31 - Integer.numberOfLeadingZeros(value)) / 7; - for (int i = 0; i < continuationBytes; ++i) { - buf.writeByte(((byte) ((value & 0x7F) | 0x80))); + int bytes = varIntBytes(value); + // Optimization: focus on 1-3 byte VarInts as they are the most common + if (bytes == 1) { + buf.writeByte(value & 0x7f); + } else if (bytes == 2) { + int w = (value & 0x7F | 0x80) << 8 | (value >>> 7); + buf.writeShort(w); + } else if (bytes == 3) { + int w = (value & 0x7F | 0x80) << 16 | ((value >>> 7) & 0x7F | 0x80) << 8 | (value >>> 14); + buf.writeMedium(w); + } else { + // 4 and 5 byte VarInts aren't common so split those cases off + writeVarIntUncommon(buf, value); + } + } + + private static void writeVarIntUncommon(ByteBuf buf, int value) { + while (true) { + if ((value & 0xFFFFFF80) == 0) { + buf.writeByte(value); + return; + } + + buf.writeByte(value & 0x7F | 0x80); value >>>= 7; } - buf.writeByte((byte) value); } public static String readString(ByteBuf buf) { diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftVarintLengthEncoder.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftVarintLengthEncoder.java index 867ea9b7f..94bfa84d0 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftVarintLengthEncoder.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftVarintLengthEncoder.java @@ -43,7 +43,8 @@ public class MinecraftVarintLengthEncoder extends MessageToByteEncoder @Override protected ByteBuf allocateBuffer(ChannelHandlerContext ctx, ByteBuf msg, boolean preferDirect) throws Exception { - int anticipatedRequiredCapacity = 5 + msg.readableBytes(); + int anticipatedRequiredCapacity = ProtocolUtils.varIntBytes(msg.readableBytes()) + + msg.readableBytes(); return IS_JAVA_CIPHER ? ctx.alloc().heapBuffer(anticipatedRequiredCapacity) : ctx.alloc().directBuffer(anticipatedRequiredCapacity); From fb3f21abc6362bbed6e343ef559fb3d5d25342d1 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Thu, 6 May 2021 00:48:19 -0400 Subject: [PATCH 18/28] More bitshifting magic --- .../velocitypowered/proxy/protocol/ProtocolUtils.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java index 1f696bb7a..c7e25f528 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java @@ -120,14 +120,13 @@ public enum ProtocolUtils { * @param value the integer to write */ public static void writeVarInt(ByteBuf buf, int value) { - int bytes = varIntBytes(value); // Optimization: focus on 1-3 byte VarInts as they are the most common - if (bytes == 1) { - buf.writeByte(value & 0x7f); - } else if (bytes == 2) { + if ((value & (0xFFFFFFFF << 7)) == 0) { + buf.writeByte(value); + } else if ((value & (0xFFFFFFFF << 14)) == 0) { int w = (value & 0x7F | 0x80) << 8 | (value >>> 7); buf.writeShort(w); - } else if (bytes == 3) { + } else if ((value & (0xFFFFFFFF << 21)) == 0) { int w = (value & 0x7F | 0x80) << 16 | ((value >>> 7) & 0x7F | 0x80) << 8 | (value >>> 14); buf.writeMedium(w); } else { From bbc53da87ebe1f70d53170dc55c8d0d82bd21120 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Thu, 6 May 2021 16:10:56 -0400 Subject: [PATCH 19/28] Create FUNDING.yml --- .github/FUNDING.yml | 1 + 1 file changed, 1 insertion(+) create mode 100644 .github/FUNDING.yml diff --git a/.github/FUNDING.yml b/.github/FUNDING.yml new file mode 100644 index 000000000..52d913562 --- /dev/null +++ b/.github/FUNDING.yml @@ -0,0 +1 @@ +github: astei From 3fdacd30d167dfbade07a33e7234c38aa99f2fbc Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Thu, 6 May 2021 16:43:40 -0400 Subject: [PATCH 20/28] Explain why Player#sendPluginMessage may not do what some people think it ought to do --- .../java/com/velocitypowered/api/proxy/Player.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/api/src/main/java/com/velocitypowered/api/proxy/Player.java b/api/src/main/java/com/velocitypowered/api/proxy/Player.java index d03e532b7..de31ae71d 100644 --- a/api/src/main/java/com/velocitypowered/api/proxy/Player.java +++ b/api/src/main/java/com/velocitypowered/api/proxy/Player.java @@ -9,6 +9,7 @@ package com.velocitypowered.api.proxy; import com.velocitypowered.api.command.CommandSource; import com.velocitypowered.api.event.player.PlayerResourcePackStatusEvent; +import com.velocitypowered.api.proxy.messages.ChannelIdentifier; import com.velocitypowered.api.proxy.messages.ChannelMessageSink; import com.velocitypowered.api.proxy.messages.ChannelMessageSource; import com.velocitypowered.api.proxy.player.PlayerSettings; @@ -226,4 +227,16 @@ public interface Player extends CommandSource, Identified, InboundConnection, * @param hash the SHA-1 hash value for the resource pack */ void sendResourcePack(String url, byte[] hash); + + /** + * @inheritDoc + * + * Note that this method does not send a plugin message to the server the player + * is connected to. You should only use this method if you are trying to communicate + * with a mod that is installed on the player's client. To send a plugin message to the server + * from the player, you should use the equivalent method on the instance returned by + * {@link #getCurrentServer()}. + */ + @Override + boolean sendPluginMessage(ChannelIdentifier identifier, byte[] data); } From 3d9a16689230152972dfd5c4a783bdac57ca4235 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Thu, 6 May 2021 16:46:43 -0400 Subject: [PATCH 21/28] Fix Checkstyle --- api/src/main/java/com/velocitypowered/api/proxy/Player.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/src/main/java/com/velocitypowered/api/proxy/Player.java b/api/src/main/java/com/velocitypowered/api/proxy/Player.java index de31ae71d..540adc75e 100644 --- a/api/src/main/java/com/velocitypowered/api/proxy/Player.java +++ b/api/src/main/java/com/velocitypowered/api/proxy/Player.java @@ -229,13 +229,13 @@ public interface Player extends CommandSource, Identified, InboundConnection, void sendResourcePack(String url, byte[] hash); /** - * @inheritDoc - * * Note that this method does not send a plugin message to the server the player * is connected to. You should only use this method if you are trying to communicate * with a mod that is installed on the player's client. To send a plugin message to the server * from the player, you should use the equivalent method on the instance returned by * {@link #getCurrentServer()}. + * + * @inheritDoc */ @Override boolean sendPluginMessage(ChannelIdentifier identifier, byte[] data); From 37a4199d4338e859b23bec7aed0f54930d815dfe Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Thu, 6 May 2021 19:56:45 -0400 Subject: [PATCH 22/28] Combine VarInt prefix encoding with compression This saves us a memory copy in the common "there is no need to compress this packet" case. --- .../proxy/connection/MinecraftConnection.java | 10 ++++---- ... MinecraftCompressorAndLengthEncoder.java} | 24 +++++++++++++++---- 2 files changed, 26 insertions(+), 8 deletions(-) rename proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/{MinecraftCompressEncoder.java => MinecraftCompressorAndLengthEncoder.java} (76%) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/connection/MinecraftConnection.java b/proxy/src/main/java/com/velocitypowered/proxy/connection/MinecraftConnection.java index 4d6318968..ba172db42 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/connection/MinecraftConnection.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/connection/MinecraftConnection.java @@ -36,12 +36,13 @@ import com.velocitypowered.proxy.VelocityServer; import com.velocitypowered.proxy.connection.client.HandshakeSessionHandler; import com.velocitypowered.proxy.connection.client.LoginSessionHandler; import com.velocitypowered.proxy.connection.client.StatusSessionHandler; +import com.velocitypowered.proxy.network.Connections; import com.velocitypowered.proxy.protocol.MinecraftPacket; import com.velocitypowered.proxy.protocol.StateRegistry; import com.velocitypowered.proxy.protocol.netty.MinecraftCipherDecoder; import com.velocitypowered.proxy.protocol.netty.MinecraftCipherEncoder; import com.velocitypowered.proxy.protocol.netty.MinecraftCompressDecoder; -import com.velocitypowered.proxy.protocol.netty.MinecraftCompressEncoder; +import com.velocitypowered.proxy.protocol.netty.MinecraftCompressorAndLengthEncoder; import com.velocitypowered.proxy.protocol.netty.MinecraftDecoder; import com.velocitypowered.proxy.protocol.netty.MinecraftEncoder; import com.velocitypowered.proxy.util.except.QuietDecoderException; @@ -402,8 +403,8 @@ public class MinecraftConnection extends ChannelInboundHandlerAdapter { } else { MinecraftCompressDecoder decoder = (MinecraftCompressDecoder) channel.pipeline() .get(COMPRESSION_DECODER); - MinecraftCompressEncoder encoder = (MinecraftCompressEncoder) channel.pipeline() - .get(COMPRESSION_ENCODER); + MinecraftCompressorAndLengthEncoder encoder = + (MinecraftCompressorAndLengthEncoder) channel.pipeline().get(COMPRESSION_ENCODER); if (decoder != null && encoder != null) { decoder.setThreshold(threshold); encoder.setThreshold(threshold); @@ -411,9 +412,10 @@ public class MinecraftConnection extends ChannelInboundHandlerAdapter { int level = server.getConfiguration().getCompressionLevel(); VelocityCompressor compressor = Natives.compress.get().create(level); - encoder = new MinecraftCompressEncoder(threshold, compressor); + encoder = new MinecraftCompressorAndLengthEncoder(threshold, compressor); decoder = new MinecraftCompressDecoder(threshold, compressor); + channel.pipeline().remove(FRAME_ENCODER); channel.pipeline().addBefore(MINECRAFT_DECODER, COMPRESSION_DECODER, decoder); channel.pipeline().addBefore(MINECRAFT_ENCODER, COMPRESSION_ENCODER, encoder); } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressEncoder.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressorAndLengthEncoder.java similarity index 76% rename from proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressEncoder.java rename to proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressorAndLengthEncoder.java index f6cb495ca..6684ee35d 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressEncoder.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressorAndLengthEncoder.java @@ -23,13 +23,14 @@ import com.velocitypowered.proxy.protocol.ProtocolUtils; import io.netty.buffer.ByteBuf; import io.netty.channel.ChannelHandlerContext; import io.netty.handler.codec.MessageToByteEncoder; +import java.util.zip.DataFormatException; -public class MinecraftCompressEncoder extends MessageToByteEncoder { +public class MinecraftCompressorAndLengthEncoder extends MessageToByteEncoder { private int threshold; private final VelocityCompressor compressor; - public MinecraftCompressEncoder(int threshold, VelocityCompressor compressor) { + public MinecraftCompressorAndLengthEncoder(int threshold, VelocityCompressor compressor) { this.threshold = threshold; this.compressor = compressor; } @@ -39,16 +40,31 @@ public class MinecraftCompressEncoder extends MessageToByteEncoder { int uncompressed = msg.readableBytes(); if (uncompressed < threshold) { // Under the threshold, there is nothing to do. + ProtocolUtils.writeVarInt(out, uncompressed + 1); ProtocolUtils.writeVarInt(out, 0); out.writeBytes(msg); } else { - ProtocolUtils.writeVarInt(out, uncompressed); + handleCompressed(ctx, msg, out); + } + } + + private void handleCompressed(ChannelHandlerContext ctx, ByteBuf msg, ByteBuf out) + throws DataFormatException { + int uncompressed = msg.readableBytes(); + ByteBuf tmpBuf = MoreByteBufUtils.preferredBuffer(ctx.alloc(), compressor, uncompressed - 1); + try { + ProtocolUtils.writeVarInt(tmpBuf, uncompressed); ByteBuf compatibleIn = MoreByteBufUtils.ensureCompatible(ctx.alloc(), compressor, msg); try { - compressor.deflate(compatibleIn, out); + compressor.deflate(compatibleIn, tmpBuf); } finally { compatibleIn.release(); } + + ProtocolUtils.writeVarInt(out, tmpBuf.readableBytes()); + out.writeBytes(tmpBuf); + } finally { + tmpBuf.release(); } } From 9318fe3dcab44ff9121a422eeeacd72f45efac7b Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Thu, 6 May 2021 21:08:57 -0400 Subject: [PATCH 23/28] Update link to Adventure documentation --- api/build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/build.gradle b/api/build.gradle index 273f61add..79f12e478 100644 --- a/api/build.gradle +++ b/api/build.gradle @@ -93,7 +93,7 @@ javadoc { 'https://google.github.io/guava/releases/25.1-jre/api/docs/', 'https://google.github.io/guice/api-docs/4.2/javadoc/', 'https://docs.oracle.com/javase/8/docs/api/', - 'https://jd.adventure.kyori.net/api/4.0.0/' + 'https://jd.adventure.kyori.net/api/4.7.0/' ) // Disable the crazy super-strict doclint tool in Java 8 From a8e0516d18b996b3a6aea80de97e017daa6e38a1 Mon Sep 17 00:00:00 2001 From: Leymooo Date: Fri, 7 May 2021 19:36:30 +0300 Subject: [PATCH 24/28] Also do not copy memory in case when packet needs to compress --- .../proxy/protocol/ProtocolUtils.java | 10 +++++ .../MinecraftCompressorAndLengthEncoder.java | 45 ++++++++++--------- .../netty/MinecraftVarintLengthEncoder.java | 2 +- .../proxy/protocol/ProtocolUtilsTest.java | 14 ++++++ 4 files changed, 49 insertions(+), 22 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java index c7e25f528..a40598e43 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java @@ -147,6 +147,16 @@ public enum ProtocolUtils { } } + /** + * Writes all integers as 3 bytes Minecraft-style VarInt to the specified {@code buf}. + * @param buf the buffer to read from + * @param value the integer to write + */ + public static void writeVarIntAs3Bytes(ByteBuf buf, int value) { + int w = (value & 0x7F | 0x80) << 16 | ((value >>> 7) & 0x7F | 0x80) << 8 | (value >>> 14); + buf.writeMedium(w); + } + public static String readString(ByteBuf buf) { return readString(buf, DEFAULT_MAX_STRING_SIZE); } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressorAndLengthEncoder.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressorAndLengthEncoder.java index 6684ee35d..73cea902b 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressorAndLengthEncoder.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressorAndLengthEncoder.java @@ -17,6 +17,8 @@ package com.velocitypowered.proxy.protocol.netty; +import static com.velocitypowered.proxy.protocol.netty.MinecraftVarintLengthEncoder.IS_JAVA_CIPHER; + import com.velocitypowered.natives.compression.VelocityCompressor; import com.velocitypowered.natives.util.MoreByteBufUtils; import com.velocitypowered.proxy.protocol.ProtocolUtils; @@ -50,36 +52,37 @@ public class MinecraftCompressorAndLengthEncoder extends MessageToByteEncoder { public static final MinecraftVarintLengthEncoder INSTANCE = new MinecraftVarintLengthEncoder(); - private static final boolean IS_JAVA_CIPHER = Natives.cipher.get() == JavaVelocityCipher.FACTORY; + public static final boolean IS_JAVA_CIPHER = Natives.cipher.get() == JavaVelocityCipher.FACTORY; private MinecraftVarintLengthEncoder() { } diff --git a/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java b/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java index 57cd8f7da..58530a4a3 100644 --- a/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java +++ b/proxy/src/test/java/com/velocitypowered/proxy/protocol/ProtocolUtilsTest.java @@ -71,6 +71,20 @@ public class ProtocolUtilsTest { assertEquals(test, ProtocolUtils.readVarIntSafely(buf)); } + @Test + void test3Bytes() { + ByteBuf buf = Unpooled.buffer(5); + for (int i = 0; i < 2097152; i += 31) { + writeReadTest3Bytes(buf, i); + } + } + + private void writeReadTest3Bytes(ByteBuf buf, int test) { + buf.clear(); + ProtocolUtils.writeVarIntAs3Bytes(buf, test); + assertEquals(test, ProtocolUtils.readVarInt(buf)); + } + @Test void testBytesWrittenAtBitBoundaries() { ByteBuf varintNew = Unpooled.buffer(5); From 6369a95ec99f4a1146ff3c38f1321770331dd4f5 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Sat, 8 May 2021 18:40:23 -0400 Subject: [PATCH 25/28] Readd safe and slow compression handling and hide it behind a system property --- .../MinecraftCompressorAndLengthEncoder.java | 44 +++++++++++++++++-- 1 file changed, 40 insertions(+), 4 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressorAndLengthEncoder.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressorAndLengthEncoder.java index 73cea902b..34d247bc7 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressorAndLengthEncoder.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressorAndLengthEncoder.java @@ -29,6 +29,9 @@ import java.util.zip.DataFormatException; public class MinecraftCompressorAndLengthEncoder extends MessageToByteEncoder { + private static final boolean MUST_USE_SAFE_AND_SLOW_COMPRESSION_HANDLING = + Boolean.getBoolean("velocity.increased-compression-cap"); + private int threshold; private final VelocityCompressor compressor; @@ -46,29 +49,62 @@ public class MinecraftCompressorAndLengthEncoder extends MessageToByteEncoder= 1 << 21) { + throw new DataFormatException("The server sent a very large (over 2MiB compressed) packet. " + + "Please restart Velocity with the JVM flag -Dvelocity.increased-compression-cap=true " + + "to fix this issue."); + } int writerIndex = out.writerIndex(); int packetLength = out.readableBytes() - 3; out.writerIndex(0); - ProtocolUtils.writeVarIntAs3Bytes(out, packetLength); //Rewrite packet length + ProtocolUtils.writeVarIntAs3Bytes(out, packetLength); // Rewrite packet length out.writerIndex(writerIndex); } + private void handleCompressedSafe(ChannelHandlerContext ctx, ByteBuf msg, ByteBuf out) + throws DataFormatException { + int uncompressed = msg.readableBytes(); + ByteBuf tmpBuf = MoreByteBufUtils.preferredBuffer(ctx.alloc(), compressor, uncompressed - 1); + try { + ProtocolUtils.writeVarInt(tmpBuf, uncompressed); + ByteBuf compatibleIn = MoreByteBufUtils.ensureCompatible(ctx.alloc(), compressor, msg); + try { + compressor.deflate(compatibleIn, tmpBuf); + } finally { + compatibleIn.release(); + } + + ProtocolUtils.writeVarInt(out, tmpBuf.readableBytes()); + out.writeBytes(tmpBuf); + } finally { + tmpBuf.release(); + } + } + @Override protected ByteBuf allocateBuffer(ChannelHandlerContext ctx, ByteBuf msg, boolean preferDirect) throws Exception { From 150fd9a9cf239b769fe46f607c95a85a6c197377 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Sat, 8 May 2021 23:26:43 -0400 Subject: [PATCH 26/28] Add highly-optimized VarInt writing method --- .../proxy/protocol/ProtocolUtils.java | 30 ++++++++----------- .../MinecraftCompressorAndLengthEncoder.java | 4 +-- .../proxy/protocol/ProtocolUtilsTest.java | 2 +- 3 files changed, 16 insertions(+), 20 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java index a40598e43..ee3482c9d 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java @@ -120,7 +120,7 @@ public enum ProtocolUtils { * @param value the integer to write */ public static void writeVarInt(ByteBuf buf, int value) { - // Optimization: focus on 1-3 byte VarInts as they are the most common + // See https://steinborn.me/posts/performance/how-fast-can-you-write-a-varint/ if ((value & (0xFFFFFFFF << 7)) == 0) { buf.writeByte(value); } else if ((value & (0xFFFFFFFF << 14)) == 0) { @@ -129,30 +129,26 @@ public enum ProtocolUtils { } else if ((value & (0xFFFFFFFF << 21)) == 0) { int w = (value & 0x7F | 0x80) << 16 | ((value >>> 7) & 0x7F | 0x80) << 8 | (value >>> 14); buf.writeMedium(w); + } else if ((value & (0xFFFFFFFF << 28)) == 0) { + int w = (value & 0x7F | 0x80) << 24 | (((value >>> 7) & 0x7F | 0x80) << 16) + | ((value >>> 14) & 0x7F | 0x80) << 8 | (value >>> 21); + buf.writeInt(w); } else { - // 4 and 5 byte VarInts aren't common so split those cases off - writeVarIntUncommon(buf, value); - } - } - - private static void writeVarIntUncommon(ByteBuf buf, int value) { - while (true) { - if ((value & 0xFFFFFF80) == 0) { - buf.writeByte(value); - return; - } - - buf.writeByte(value & 0x7F | 0x80); - value >>>= 7; + int w = (value & 0x7F | 0x80) << 24 | ((value >>> 7) & 0x7F | 0x80) << 16 + | ((value >>> 14) & 0x7F | 0x80) << 8 | ((value >>> 21) & 0x7F | 0x80); + buf.writeInt(w); + buf.writeByte(value >>> 28); } } /** - * Writes all integers as 3 bytes Minecraft-style VarInt to the specified {@code buf}. + * Writes the specified {@code value} as a 21-bit Minecraft VarInt to the specified {@code buf}. + * The upper 11 bits will be discarded. * @param buf the buffer to read from * @param value the integer to write */ - public static void writeVarIntAs3Bytes(ByteBuf buf, int value) { + public static void write21BitVarInt(ByteBuf buf, int value) { + // See https://steinborn.me/posts/performance/how-fast-can-you-write-a-varint/ int w = (value & 0x7F | 0x80) << 16 | ((value >>> 7) & 0x7F | 0x80) << 8 | (value >>> 14); buf.writeMedium(w); } diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressorAndLengthEncoder.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressorAndLengthEncoder.java index 34d247bc7..28d005161 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressorAndLengthEncoder.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/netty/MinecraftCompressorAndLengthEncoder.java @@ -61,7 +61,7 @@ public class MinecraftCompressorAndLengthEncoder extends MessageToByteEncoder Date: Sun, 9 May 2021 02:57:01 -0400 Subject: [PATCH 27/28] Improve writeVarInt inlining by peeling the two most common cases --- .../proxy/protocol/ProtocolUtils.java | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java index ee3482c9d..7ae34b9db 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java @@ -120,6 +120,19 @@ public enum ProtocolUtils { * @param value the integer to write */ public static void writeVarInt(ByteBuf buf, int value) { + // Peel the one and two byte count cases explicitly as they are the most common VarInt sizes + // that the proxy will write, to improve inlining. + if ((value & (0xFFFFFFFF << 7)) == 0) { + buf.writeByte(value); + } else if ((value & (0xFFFFFFFF << 14)) == 0) { + int w = (value & 0x7F | 0x80) << 8 | (value >>> 7); + buf.writeShort(w); + } else { + writeVarIntFull(buf, value); + } + } + + private static void writeVarIntFull(ByteBuf buf, int value) { // See https://steinborn.me/posts/performance/how-fast-can-you-write-a-varint/ if ((value & (0xFFFFFFFF << 7)) == 0) { buf.writeByte(value); @@ -135,7 +148,7 @@ public enum ProtocolUtils { buf.writeInt(w); } else { int w = (value & 0x7F | 0x80) << 24 | ((value >>> 7) & 0x7F | 0x80) << 16 - | ((value >>> 14) & 0x7F | 0x80) << 8 | ((value >>> 21) & 0x7F | 0x80); + | ((value >>> 14) & 0x7F | 0x80) | ((value >>> 21) & 0x7F | 0x80); buf.writeInt(w); buf.writeByte(value >>> 28); } From 11ed4b46e4b19a22a10063b03b41be0ae7d55a67 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Sun, 9 May 2021 02:57:52 -0400 Subject: [PATCH 28/28] whoops --- .../java/com/velocitypowered/proxy/protocol/ProtocolUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java index 7ae34b9db..53daae299 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/protocol/ProtocolUtils.java @@ -148,7 +148,7 @@ public enum ProtocolUtils { buf.writeInt(w); } else { int w = (value & 0x7F | 0x80) << 24 | ((value >>> 7) & 0x7F | 0x80) << 16 - | ((value >>> 14) & 0x7F | 0x80) | ((value >>> 21) & 0x7F | 0x80); + | ((value >>> 14) & 0x7F | 0x80) << 8 | ((value >>> 21) & 0x7F | 0x80); buf.writeInt(w); buf.writeByte(value >>> 28); }