From d13ce24507184ea4058bc77d406594d178162a39 Mon Sep 17 00:00:00 2001 From: Dan Mulloy Date: Sat, 8 Jun 2024 14:09:23 -0500 Subject: [PATCH] Stop caching constructor parameters Addresses #2990 --- .../protocol/events/PacketContainer.java | 7 +- .../protocol/injector/StructureCache.java | 118 ++++++++++-------- ...acketCreator.java => InstanceCreator.java} | 39 +++--- .../wrappers/WrappedTeamParameters.java | 3 +- 4 files changed, 88 insertions(+), 79 deletions(-) rename src/main/java/com/comphenix/protocol/reflect/instances/{PacketCreator.java => InstanceCreator.java} (74%) diff --git a/src/main/java/com/comphenix/protocol/events/PacketContainer.java b/src/main/java/com/comphenix/protocol/events/PacketContainer.java index 19ee953b..ba50ca72 100644 --- a/src/main/java/com/comphenix/protocol/events/PacketContainer.java +++ b/src/main/java/com/comphenix/protocol/events/PacketContainer.java @@ -31,6 +31,7 @@ import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.function.Function; +import javax.annotation.Nullable; import com.comphenix.protocol.PacketType; import com.comphenix.protocol.injector.StructureCache; @@ -58,10 +59,10 @@ import com.comphenix.protocol.utility.MinecraftReflection; import com.comphenix.protocol.utility.MinecraftVersion; import com.comphenix.protocol.wrappers.Converters; import com.comphenix.protocol.wrappers.WrappedStreamCodec; + import com.google.common.collect.Sets; import io.netty.buffer.ByteBuf; import io.netty.util.ReferenceCountUtil; -import javax.annotation.Nullable; /** * Represents a Minecraft packet indirectly. @@ -80,7 +81,7 @@ public class PacketContainer extends AbstractStructure implements Serializable { // Used to clone packets private static final AggregateCloner DEEP_CLONER = AggregateCloner .newBuilder() - .instanceProvider(StructureCache::newPacket) + .instanceProvider(StructureCache::newInstance) .andThen(BukkitCloner.class) .andThen(ImmutableDetector.class) .andThen(JavaOptionalCloner.class) @@ -91,7 +92,7 @@ public class PacketContainer extends AbstractStructure implements Serializable { private static final AggregateCloner SHALLOW_CLONER = AggregateCloner .newBuilder() - .instanceProvider(StructureCache::newPacket) + .instanceProvider(StructureCache::newInstance) .andThen(param -> { if (param == null) throw new IllegalArgumentException("Cannot be NULL."); diff --git a/src/main/java/com/comphenix/protocol/injector/StructureCache.java b/src/main/java/com/comphenix/protocol/injector/StructureCache.java index 65f8f7a0..cb50e3c7 100644 --- a/src/main/java/com/comphenix/protocol/injector/StructureCache.java +++ b/src/main/java/com/comphenix/protocol/injector/StructureCache.java @@ -32,7 +32,7 @@ import com.comphenix.protocol.reflect.accessors.Accessors; import com.comphenix.protocol.reflect.accessors.ConstructorAccessor; import com.comphenix.protocol.reflect.fuzzy.FuzzyMethodContract; import com.comphenix.protocol.reflect.instances.DefaultInstances; -import com.comphenix.protocol.reflect.instances.PacketCreator; +import com.comphenix.protocol.reflect.instances.InstanceCreator; import com.comphenix.protocol.utility.ByteBuddyFactory; import com.comphenix.protocol.utility.MinecraftMethods; import com.comphenix.protocol.utility.MinecraftReflection; @@ -57,7 +57,7 @@ import net.bytebuddy.matcher.ElementMatchers; public class StructureCache { // Structure modifiers - private static final Map, Supplier> PACKET_INSTANCE_CREATORS = new ConcurrentHashMap<>(); + private static final Map, Supplier> CACHED_INSTANCE_CREATORS = new ConcurrentHashMap<>(); private static final Map> STRUCTURE_MODIFIER_CACHE = new ConcurrentHashMap<>(); // packet data serializer which always returns an empty nbt tag compound @@ -67,80 +67,90 @@ public class StructureCache { private static Supplier TRICKED_DATA_SERIALIZER_BASE; private static Supplier TRICKED_DATA_SERIALIZER_JSON; + /** + * @deprecated Renamed to {@link #newInstance(Class)}. + */ + @Deprecated public static Object newPacket(Class packetClass) { - Supplier packetConstructor = PACKET_INSTANCE_CREATORS.computeIfAbsent(packetClass, packetClassKey -> { - try { - PacketCreator creator = PacketCreator.forPacket(packetClassKey); - if (creator.get() != null) { - return creator; - } - } catch (Exception ignored) { + return newInstance(packetClass); + } + + public static Object newInstance(Class clazz) { + Supplier creator = CACHED_INSTANCE_CREATORS.computeIfAbsent(clazz, StructureCache::determineBestCreator); + return creator.get(); + } + + static Supplier determineBestCreator(Class clazz) { + try { + InstanceCreator creator = InstanceCreator.forClass(clazz); + if (creator.get() != null) { + return creator; } + } catch (Exception ignored) { + } - WrappedStreamCodec streamCodec = PacketRegistry.getStreamCodec(packetClassKey); + WrappedStreamCodec streamCodec = PacketRegistry.getStreamCodec(clazz); - // use the new stream codec for versions above 1.20.5 if possible - if (streamCodec != null && tryInitTrickDataSerializer()) { + // use the new stream codec for versions above 1.20.5 if possible + if (streamCodec != null && tryInitTrickDataSerializer()) { + try { + // first try with the base accessor + Object serializer = TRICKED_DATA_SERIALIZER_BASE.get(); + streamCodec.decode(serializer); // throwaway instance, for testing + + // method is working + return () -> streamCodec.decode(serializer); + } catch (Exception ignored) { try { - // first try with the base accessor - Object serializer = TRICKED_DATA_SERIALIZER_BASE.get(); + // try with the json accessor + Object serializer = TRICKED_DATA_SERIALIZER_JSON.get(); streamCodec.decode(serializer); // throwaway instance, for testing // method is working return () -> streamCodec.decode(serializer); - } catch (Exception ignored) { - try { - // try with the json accessor - Object serializer = TRICKED_DATA_SERIALIZER_JSON.get(); - streamCodec.decode(serializer); // throwaway instance, for testing - - // method is working - return () -> streamCodec.decode(serializer); - } catch (Exception ignored1) { - // shrug, fall back to default behaviour - } + } catch (Exception ignored1) { + // shrug, fall back to default behaviour } } + } - // prefer construction via PacketDataSerializer constructor on 1.17 and above - if (MinecraftVersion.CAVES_CLIFFS_1.atOrAbove()) { - ConstructorAccessor serializerAccessor = Accessors.getConstructorAccessorOrNull( - packetClassKey, - MinecraftReflection.getPacketDataSerializerClass()); - if (serializerAccessor != null) { - // check if the method is possible - if (tryInitTrickDataSerializer()) { + // prefer construction via PacketDataSerializer constructor on 1.17 and above + if (MinecraftVersion.CAVES_CLIFFS_1.atOrAbove()) { + ConstructorAccessor serializerAccessor = Accessors.getConstructorAccessorOrNull( + clazz, + MinecraftReflection.getPacketDataSerializerClass()); + if (serializerAccessor != null) { + // check if the method is possible + if (tryInitTrickDataSerializer()) { + try { + // first try with the base accessor + Object serializer = TRICKED_DATA_SERIALIZER_BASE.get(); + serializerAccessor.invoke(serializer); // throwaway instance, for testing + + // method is working + return () -> serializerAccessor.invoke(serializer); + } catch (Exception ignored) { try { - // first try with the base accessor - Object serializer = TRICKED_DATA_SERIALIZER_BASE.get(); + // try with the json accessor + Object serializer = TRICKED_DATA_SERIALIZER_JSON.get(); serializerAccessor.invoke(serializer); // throwaway instance, for testing // method is working return () -> serializerAccessor.invoke(serializer); - } catch (Exception ignored) { - try { - // try with the json accessor - Object serializer = TRICKED_DATA_SERIALIZER_JSON.get(); - serializerAccessor.invoke(serializer); // throwaway instance, for testing - - // method is working - return () -> serializerAccessor.invoke(serializer); - } catch (Exception ignored1) { - // shrug, fall back to default behaviour - } + } catch (Exception ignored1) { + // shrug, fall back to default behaviour } } } } + } - // try via DefaultInstances as fallback - return () -> { - Object packetInstance = DefaultInstances.DEFAULT.create(packetClassKey); - Objects.requireNonNull(packetInstance, "Unable to create packet instance for class " + packetClassKey + " - " + tryInitTrickDataSerializer() + " - " + streamCodec); - return packetInstance; - }; - }); - return packetConstructor.get(); + // try via DefaultInstances as fallback + return () -> { + Object packetInstance = DefaultInstances.DEFAULT.create(clazz); + Objects.requireNonNull(packetInstance, "Unable to create instance for class " + clazz + " - " + tryInitTrickDataSerializer() + " - " + streamCodec); + return packetInstance; + }; } /** diff --git a/src/main/java/com/comphenix/protocol/reflect/instances/PacketCreator.java b/src/main/java/com/comphenix/protocol/reflect/instances/InstanceCreator.java similarity index 74% rename from src/main/java/com/comphenix/protocol/reflect/instances/PacketCreator.java rename to src/main/java/com/comphenix/protocol/reflect/instances/InstanceCreator.java index 3ed88561..c37a8a91 100644 --- a/src/main/java/com/comphenix/protocol/reflect/instances/PacketCreator.java +++ b/src/main/java/com/comphenix/protocol/reflect/instances/InstanceCreator.java @@ -8,30 +8,25 @@ import java.util.function.Supplier; import com.comphenix.protocol.reflect.accessors.Accessors; import com.comphenix.protocol.reflect.accessors.ConstructorAccessor; import com.comphenix.protocol.reflect.accessors.MethodAccessor; -import com.comphenix.protocol.utility.MinecraftReflection; -public final class PacketCreator implements Supplier { +public final class InstanceCreator implements Supplier { private ConstructorAccessor constructor = null; private MethodAccessor factoryMethod = null; - private Object[] params = null; + private Class[] paramTypes = null; private boolean failed = false; private final Class type; - private PacketCreator(Class type) { + private InstanceCreator(Class type) { this.type = type; } - public static PacketCreator forPacket(Class type) { + public static InstanceCreator forClass(Class type) { if (type == null) { throw new IllegalArgumentException("Type cannot be null."); } - if (!MinecraftReflection.getPacketClass().isAssignableFrom(type)) { - throw new IllegalArgumentException("Type must be a subclass of Packet."); - } - - return new PacketCreator(type); + return new InstanceCreator(type); } private Object createInstance(Class clazz) { @@ -42,8 +37,18 @@ public final class PacketCreator implements Supplier { } } + private Object[] createParams(Class[] paramTypes) { + Object[] params = new Object[paramTypes.length]; + for (int i = 0; i < paramTypes.length; i++) { + params[i] = createInstance(paramTypes[i]); + } + return params; + } + @Override public Object get() { + Object[] params = paramTypes != null ? createParams(paramTypes) : null; + if (constructor != null) { return constructor.invoke(params); } @@ -65,16 +70,13 @@ public final class PacketCreator implements Supplier { continue; } - Object[] testParams = new Object[paramTypes.length]; - for (int i = 0; i < paramTypes.length; i++) { - testParams[i] = createInstance(paramTypes[i]); - } + Object[] testParams = createParams(paramTypes); try { result = testCtor.newInstance(testParams); minCount = paramTypes.length; this.constructor = Accessors.getConstructorAccessor(testCtor); - this.params = testParams; + this.paramTypes = paramTypes; } catch (Exception ignored) { } } @@ -100,16 +102,13 @@ public final class PacketCreator implements Supplier { continue; } - Object[] testParams = new Object[paramTypes.length]; - for (int i = 0; i < paramTypes.length; i++) { - testParams[i] = createInstance(paramTypes[i]); - } + Object[] testParams = createParams(paramTypes); try { result = testMethod.invoke(null, testParams); minCount = paramTypes.length; this.factoryMethod = Accessors.getMethodAccessor(testMethod); - this.params = testParams; + this.paramTypes = paramTypes; } catch (Exception ignored) { } } diff --git a/src/main/java/com/comphenix/protocol/wrappers/WrappedTeamParameters.java b/src/main/java/com/comphenix/protocol/wrappers/WrappedTeamParameters.java index a9354163..d0c59fb6 100644 --- a/src/main/java/com/comphenix/protocol/wrappers/WrappedTeamParameters.java +++ b/src/main/java/com/comphenix/protocol/wrappers/WrappedTeamParameters.java @@ -154,8 +154,7 @@ public class WrappedTeamParameters extends AbstractWrapper { Preconditions.checkNotNull(collisionRule, "Collision rule not set"); Preconditions.checkNotNull(color, "Color not set"); - // Not technically a packet, but it has a PacketDataSerializer constructor, so it works fine - Object handle = StructureCache.newPacket(getNmsClassOrThrow()); + Object handle = StructureCache.newInstance(getNmsClassOrThrow()); WrappedTeamParameters wrapped = new WrappedTeamParameters(handle); wrapped.writeComponent(0, displayName);