From 0ccfc595a48f7878eaab072841fc86009bc98020 Mon Sep 17 00:00:00 2001 From: Shane Freeder Date: Wed, 6 Sep 2017 21:18:36 +0100 Subject: [PATCH] Cache generated EventExecutors (fixes #786) the first 'major' change in this PR is to cache the generated event executrs from the ASM class, by doing this we only generate a single class for every method that we need an executor for, thus reducing the number of classes that are needed, especially in cases where plugins re/unregister events all the time. The second change is to modify the generated classloader map, generated classloaders are not held against the plugin itself but the classloader that the event is declared in, the implication here is that we cannot drop generated classloaders when a plugin disable, and so we use a guava weak-key'd hashmap, downfall here is that classes won't be GC'd until guava drops the generated classloader, however the first change should deal with most of the grunt. --- .../Use-ASM-for-event-executors.patch | 43 +++++++++++++++---- 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/Spigot-API-Patches/Use-ASM-for-event-executors.patch b/Spigot-API-Patches/Use-ASM-for-event-executors.patch index 87f209df00..86d9dae2af 100644 --- a/Spigot-API-Patches/Use-ASM-for-event-executors.patch +++ b/Spigot-API-Patches/Use-ASM-for-event-executors.patch @@ -203,7 +203,7 @@ index 00000000..6941d9fb +} diff --git a/src/main/java/com/destroystokyo/paper/event/executor/asm/SafeClassDefiner.java b/src/main/java/com/destroystokyo/paper/event/executor/asm/SafeClassDefiner.java new file mode 100644 -index 00000000..776a9a03 +index 00000000..1473ff8c --- /dev/null +++ b/src/main/java/com/destroystokyo/paper/event/executor/asm/SafeClassDefiner.java @@ -0,0 +0,0 @@ @@ -214,6 +214,7 @@ index 00000000..776a9a03 + +import com.google.common.base.Preconditions; + ++import com.google.common.collect.MapMaker; +import org.objectweb.asm.Type; + +public class SafeClassDefiner implements ClassDefiner { @@ -221,7 +222,7 @@ index 00000000..776a9a03 + + private SafeClassDefiner() {} + -+ private final ConcurrentMap loaders = new ConcurrentHashMap<>(); ++ private final ConcurrentMap loaders = new MapMaker().weakKeys().makeMap(); + + @Override + public Class defineClass(ClassLoader parentLoader, String name, byte[] data) { @@ -309,7 +310,7 @@ index 00000000..62acbf82 + } +} diff --git a/src/main/java/org/bukkit/plugin/EventExecutor.java b/src/main/java/org/bukkit/plugin/EventExecutor.java -index 3b2c99ea..f9316d65 100644 +index 3b2c99ea..b45b6c1c 100644 --- a/src/main/java/org/bukkit/plugin/EventExecutor.java +++ b/src/main/java/org/bukkit/plugin/EventExecutor.java @@ -0,0 +0,0 @@ import org.bukkit.event.Event; @@ -319,6 +320,10 @@ index 3b2c99ea..f9316d65 100644 +// Paper start +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; ++import java.util.HashMap; ++import java.util.concurrent.ConcurrentHashMap; ++import java.util.concurrent.ConcurrentMap; ++import java.util.function.Function; + +import com.destroystokyo.paper.event.executor.MethodHandleEventExecutor; +import com.destroystokyo.paper.event.executor.StaticMethodHandleEventExecutor; @@ -334,6 +339,24 @@ index 3b2c99ea..f9316d65 100644 public void execute(Listener listener, Event event) throws EventException; + + // Paper start ++ ConcurrentMap> eventExecutorMap = new ConcurrentHashMap>() { ++ @Override ++ public Class computeIfAbsent(Method key, Function> mappingFunction) { ++ Class executorClass = get(key); ++ if (executorClass != null) ++ return executorClass; ++ ++ //noinspection SynchronizationOnLocalVariableOrMethodParameter ++ synchronized (key) { ++ executorClass = get(key); ++ if (executorClass != null) ++ return executorClass; ++ ++ return super.computeIfAbsent(key, mappingFunction); ++ } ++ } ++ }; ++ + public static EventExecutor create(Method m, Class eventClass) { + Preconditions.checkNotNull(m, "Null method"); + Preconditions.checkArgument(m.getParameterCount() != 0, "Incorrect number of arguments %s", m.getParameterCount()); @@ -341,12 +364,16 @@ index 3b2c99ea..f9316d65 100644 + ClassDefiner definer = ClassDefiner.getInstance(); + if (Modifier.isStatic(m.getModifiers())) { + return new StaticMethodHandleEventExecutor(eventClass, m); -+ } if (definer.isBypassAccessChecks() || Modifier.isPublic(m.getDeclaringClass().getModifiers()) && Modifier.isPublic(m.getModifiers())) { -+ String name = ASMEventExecutorGenerator.generateName(); -+ byte[] classData = ASMEventExecutorGenerator.generateEventExecutor(m, name); -+ Class c = definer.defineClass(m.getDeclaringClass().getClassLoader(), name, classData).asSubclass(EventExecutor.class); ++ } else if (definer.isBypassAccessChecks() || Modifier.isPublic(m.getDeclaringClass().getModifiers()) && Modifier.isPublic(m.getModifiers())) { ++ // get the existing generated EventExecutor class for the Method or generate one ++ Class executorClass = eventExecutorMap.computeIfAbsent(m, (__) -> { ++ String name = ASMEventExecutorGenerator.generateName(); ++ byte[] classData = ASMEventExecutorGenerator.generateEventExecutor(m, name); ++ return definer.defineClass(m.getDeclaringClass().getClassLoader(), name, classData).asSubclass(EventExecutor.class); ++ }); ++ + try { -+ EventExecutor asmExecutor = c.newInstance(); ++ EventExecutor asmExecutor = executorClass.newInstance(); + // Define a wrapper to conform to bukkit stupidity (passing in events that don't match and wrapper exception) + return new EventExecutor() { + @Override