Skip to content

Commit d13ce24

Browse files
dmulloy2dmulloy2
committed
Stop caching constructor parameters
Addresses #2990
1 parent b6a7d5f commit d13ce24

File tree

4 files changed

+88
-79
lines changed

4 files changed

+88
-79
lines changed

‎src/main/java/com/comphenix/protocol/events/PacketContainer.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import java.util.concurrent.ConcurrentHashMap;
3232
import java.util.concurrent.ConcurrentMap;
3333
import java.util.function.Function;
34+
import javax.annotation.Nullable;
3435

3536
import com.comphenix.protocol.PacketType;
3637
import com.comphenix.protocol.injector.StructureCache;
@@ -58,10 +59,10 @@
5859
import com.comphenix.protocol.utility.MinecraftVersion;
5960
import com.comphenix.protocol.wrappers.Converters;
6061
import com.comphenix.protocol.wrappers.WrappedStreamCodec;
62+
6163
import com.google.common.collect.Sets;
6264
import io.netty.buffer.ByteBuf;
6365
import io.netty.util.ReferenceCountUtil;
64-
import javax.annotation.Nullable;
6566

6667
/**
6768
* Represents a Minecraft packet indirectly.
@@ -80,7 +81,7 @@ public class PacketContainer extends AbstractStructure implements Serializable {
8081
// Used to clone packets
8182
private static final AggregateCloner DEEP_CLONER = AggregateCloner
8283
.newBuilder()
83-
.instanceProvider(StructureCache::newPacket)
84+
.instanceProvider(StructureCache::newInstance)
8485
.andThen(BukkitCloner.class)
8586
.andThen(ImmutableDetector.class)
8687
.andThen(JavaOptionalCloner.class)
@@ -91,7 +92,7 @@ public class PacketContainer extends AbstractStructure implements Serializable {
9192

9293
private static final AggregateCloner SHALLOW_CLONER = AggregateCloner
9394
.newBuilder()
94-
.instanceProvider(StructureCache::newPacket)
95+
.instanceProvider(StructureCache::newInstance)
9596
.andThen(param -> {
9697
if (param == null)
9798
throw new IllegalArgumentException("Cannot be NULL.");

‎src/main/java/com/comphenix/protocol/injector/StructureCache.java‎

Lines changed: 64 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@
3232
import com.comphenix.protocol.reflect.accessors.ConstructorAccessor;
3333
import com.comphenix.protocol.reflect.fuzzy.FuzzyMethodContract;
3434
import com.comphenix.protocol.reflect.instances.DefaultInstances;
35-
import com.comphenix.protocol.reflect.instances.PacketCreator;
35+
import com.comphenix.protocol.reflect.instances.InstanceCreator;
3636
import com.comphenix.protocol.utility.ByteBuddyFactory;
3737
import com.comphenix.protocol.utility.MinecraftMethods;
3838
import com.comphenix.protocol.utility.MinecraftReflection;
@@ -57,7 +57,7 @@
5757
public class StructureCache {
5858

5959
// Structure modifiers
60-
private static final Map<Class<?>, Supplier<Object>> PACKET_INSTANCE_CREATORS = new ConcurrentHashMap<>();
60+
private static final Map<Class<?>, Supplier<Object>> CACHED_INSTANCE_CREATORS = new ConcurrentHashMap<>();
6161
private static final Map<PacketType, StructureModifier<Object>> STRUCTURE_MODIFIER_CACHE = new ConcurrentHashMap<>();
6262

6363
// packet data serializer which always returns an empty nbt tag compound
@@ -67,80 +67,90 @@ public class StructureCache {
6767
private static Supplier<Object> TRICKED_DATA_SERIALIZER_BASE;
6868
private static Supplier<Object> TRICKED_DATA_SERIALIZER_JSON;
6969

70+
/**
71+
* @deprecated Renamed to {@link #newInstance(Class)}.
72+
*/
73+
@Deprecated
7074
public static Object newPacket(Class<?> packetClass) {
71-
Supplier<Object> packetConstructor = PACKET_INSTANCE_CREATORS.computeIfAbsent(packetClass, packetClassKey -> {
72-
try {
73-
PacketCreator creator = PacketCreator.forPacket(packetClassKey);
74-
if (creator.get() != null) {
75-
return creator;
76-
}
77-
} catch (Exception ignored) {
75+
return newInstance(packetClass);
76+
}
77+
78+
public static Object newInstance(Class<?> clazz) {
79+
Supplier<Object> creator = CACHED_INSTANCE_CREATORS.computeIfAbsent(clazz, StructureCache::determineBestCreator);
80+
return creator.get();
81+
}
82+
83+
static Supplier<Object> determineBestCreator(Class<?> clazz) {
84+
try {
85+
InstanceCreator creator = InstanceCreator.forClass(clazz);
86+
if (creator.get() != null) {
87+
return creator;
7888
}
89+
} catch (Exception ignored) {
90+
}
91+
92+
WrappedStreamCodec streamCodec = PacketRegistry.getStreamCodec(clazz);
7993

80-
WrappedStreamCodec streamCodec = PacketRegistry.getStreamCodec(packetClassKey);
94+
// use the new stream codec for versions above 1.20.5 if possible
95+
if (streamCodec != null && tryInitTrickDataSerializer()) {
96+
try {
97+
// first try with the base accessor
98+
Object serializer = TRICKED_DATA_SERIALIZER_BASE.get();
99+
streamCodec.decode(serializer); // throwaway instance, for testing
81100

82-
// use the new stream codec for versions above 1.20.5 if possible
83-
if (streamCodec != null && tryInitTrickDataSerializer()) {
101+
// method is working
102+
return () -> streamCodec.decode(serializer);
103+
} catch (Exception ignored) {
84104
try {
85-
// first try with the base accessor
86-
Object serializer = TRICKED_DATA_SERIALIZER_BASE.get();
105+
// try with the json accessor
106+
Object serializer = TRICKED_DATA_SERIALIZER_JSON.get();
87107
streamCodec.decode(serializer); // throwaway instance, for testing
88108

89109
// method is working
90110
return () -> streamCodec.decode(serializer);
91-
} catch (Exception ignored) {
92-
try {
93-
// try with the json accessor
94-
Object serializer = TRICKED_DATA_SERIALIZER_JSON.get();
95-
streamCodec.decode(serializer); // throwaway instance, for testing
96-
97-
// method is working
98-
return () -> streamCodec.decode(serializer);
99-
} catch (Exception ignored1) {
100-
// shrug, fall back to default behaviour
101-
}
111+
} catch (Exception ignored1) {
112+
// shrug, fall back to default behaviour
102113
}
103114
}
115+
}
116+
117+
// prefer construction via PacketDataSerializer constructor on 1.17 and above
118+
if (MinecraftVersion.CAVES_CLIFFS_1.atOrAbove()) {
119+
ConstructorAccessor serializerAccessor = Accessors.getConstructorAccessorOrNull(
120+
clazz,
121+
MinecraftReflection.getPacketDataSerializerClass());
122+
if (serializerAccessor != null) {
123+
// check if the method is possible
124+
if (tryInitTrickDataSerializer()) {
125+
try {
126+
// first try with the base accessor
127+
Object serializer = TRICKED_DATA_SERIALIZER_BASE.get();
128+
serializerAccessor.invoke(serializer); // throwaway instance, for testing
104129

105-
// prefer construction via PacketDataSerializer constructor on 1.17 and above
106-
if (MinecraftVersion.CAVES_CLIFFS_1.atOrAbove()) {
107-
ConstructorAccessor serializerAccessor = Accessors.getConstructorAccessorOrNull(
108-
packetClassKey,
109-
MinecraftReflection.getPacketDataSerializerClass());
110-
if (serializerAccessor != null) {
111-
// check if the method is possible
112-
if (tryInitTrickDataSerializer()) {
130+
// method is working
131+
return () -> serializerAccessor.invoke(serializer);
132+
} catch (Exception ignored) {
113133
try {
114-
// first try with the base accessor
115-
Object serializer = TRICKED_DATA_SERIALIZER_BASE.get();
134+
// try with the json accessor
135+
Object serializer = TRICKED_DATA_SERIALIZER_JSON.get();
116136
serializerAccessor.invoke(serializer); // throwaway instance, for testing
117137

118138
// method is working
119139
return () -> serializerAccessor.invoke(serializer);
120-
} catch (Exception ignored) {
121-
try {
122-
// try with the json accessor
123-
Object serializer = TRICKED_DATA_SERIALIZER_JSON.get();
124-
serializerAccessor.invoke(serializer); // throwaway instance, for testing
125-
126-
// method is working
127-
return () -> serializerAccessor.invoke(serializer);
128-
} catch (Exception ignored1) {
129-
// shrug, fall back to default behaviour
130-
}
140+
} catch (Exception ignored1) {
141+
// shrug, fall back to default behaviour
131142
}
132143
}
133144
}
134145
}
146+
}
135147

136-
// try via DefaultInstances as fallback
137-
return () -> {
138-
Object packetInstance = DefaultInstances.DEFAULT.create(packetClassKey);
139-
Objects.requireNonNull(packetInstance, "Unable to create packet instance for class " + packetClassKey + " - " + tryInitTrickDataSerializer() + " - " + streamCodec);
140-
return packetInstance;
141-
};
142-
});
143-
return packetConstructor.get();
148+
// try via DefaultInstances as fallback
149+
return () -> {
150+
Object packetInstance = DefaultInstances.DEFAULT.create(clazz);
151+
Objects.requireNonNull(packetInstance, "Unable to create instance for class " + clazz + " - " + tryInitTrickDataSerializer() + " - " + streamCodec);
152+
return packetInstance;
153+
};
144154
}
145155

146156
/**

‎src/main/java/com/comphenix/protocol/reflect/instances/PacketCreator.java‎ renamed to ‎src/main/java/com/comphenix/protocol/reflect/instances/InstanceCreator.java‎

Lines changed: 19 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -8,30 +8,25 @@
88
import com.comphenix.protocol.reflect.accessors.Accessors;
99
import com.comphenix.protocol.reflect.accessors.ConstructorAccessor;
1010
import com.comphenix.protocol.reflect.accessors.MethodAccessor;
11-
import com.comphenix.protocol.utility.MinecraftReflection;
1211

13-
public final class PacketCreator implements Supplier<Object> {
12+
public final class InstanceCreator implements Supplier<Object> {
1413
private ConstructorAccessor constructor = null;
1514
private MethodAccessor factoryMethod = null;
16-
private Object[] params = null;
15+
private Class<?>[] paramTypes = null;
1716
private boolean failed = false;
1817

1918
private final Class<?> type;
2019

21-
private PacketCreator(Class<?> type) {
20+
private InstanceCreator(Class<?> type) {
2221
this.type = type;
2322
}
2423

25-
public static PacketCreator forPacket(Class<?> type) {
24+
public static InstanceCreator forClass(Class<?> type) {
2625
if (type == null) {
2726
throw new IllegalArgumentException("Type cannot be null.");
2827
}
2928

30-
if (!MinecraftReflection.getPacketClass().isAssignableFrom(type)) {
31-
throw new IllegalArgumentException("Type must be a subclass of Packet.");
32-
}
33-
34-
return new PacketCreator(type);
29+
return new InstanceCreator(type);
3530
}
3631

3732
private Object createInstance(Class<?> clazz) {
@@ -42,8 +37,18 @@ private Object createInstance(Class<?> clazz) {
4237
}
4338
}
4439

40+
private Object[] createParams(Class<?>[] paramTypes) {
41+
Object[] params = new Object[paramTypes.length];
42+
for (int i = 0; i < paramTypes.length; i++) {
43+
params[i] = createInstance(paramTypes[i]);
44+
}
45+
return params;
46+
}
47+
4548
@Override
4649
public Object get() {
50+
Object[] params = paramTypes != null ? createParams(paramTypes) : null;
51+
4752
if (constructor != null) {
4853
return constructor.invoke(params);
4954
}
@@ -65,16 +70,13 @@ public Object get() {
6570
continue;
6671
}
6772

68-
Object[] testParams = new Object[paramTypes.length];
69-
for (int i = 0; i < paramTypes.length; i++) {
70-
testParams[i] = createInstance(paramTypes[i]);
71-
}
73+
Object[] testParams = createParams(paramTypes);
7274

7375
try {
7476
result = testCtor.newInstance(testParams);
7577
minCount = paramTypes.length;
7678
this.constructor = Accessors.getConstructorAccessor(testCtor);
77-
this.params = testParams;
79+
this.paramTypes = paramTypes;
7880
} catch (Exception ignored) {
7981
}
8082
}
@@ -100,16 +102,13 @@ public Object get() {
100102
continue;
101103
}
102104

103-
Object[] testParams = new Object[paramTypes.length];
104-
for (int i = 0; i < paramTypes.length; i++) {
105-
testParams[i] = createInstance(paramTypes[i]);
106-
}
105+
Object[] testParams = createParams(paramTypes);
107106

108107
try {
109108
result = testMethod.invoke(null, testParams);
110109
minCount = paramTypes.length;
111110
this.factoryMethod = Accessors.getMethodAccessor(testMethod);
112-
this.params = testParams;
111+
this.paramTypes = paramTypes;
113112
} catch (Exception ignored) {
114113
}
115114
}

‎src/main/java/com/comphenix/protocol/wrappers/WrappedTeamParameters.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -154,8 +154,7 @@ public WrappedTeamParameters build() {
154154
Preconditions.checkNotNull(collisionRule, "Collision rule not set");
155155
Preconditions.checkNotNull(color, "Color not set");
156156

157-
// Not technically a packet, but it has a PacketDataSerializer constructor, so it works fine
158-
Object handle = StructureCache.newPacket(getNmsClassOrThrow());
157+
Object handle = StructureCache.newInstance(getNmsClassOrThrow());
159158

160159
WrappedTeamParameters wrapped = new WrappedTeamParameters(handle);
161160
wrapped.writeComponent(0, displayName);

0 commit comments

Comments
 (0)