[6.2.x] Improve cache performance (#84)

Reduces the latency of NoLoader posting in CacheCopy and CacheConcurrent modes by about 14% and 18%, respectively.

Author: PaintNinja

eventbus-test/src/test/java/net/minecraftforge/eventbus/test/map/MapTestBase.java

@@ -9,11 +9,9 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ExecutorService;
-import java.util.function.BiFunction;
 import java.util.function.Function;
-import java.util.function.Supplier;
 
-import net.minecraftforge.eventbus.InternalUtils;
+import net.minecraftforge.eventbus.internal.Cache;
 import net.minecraftforge.eventbus.test.Whitebox;
 
 public class MapTestBase {
@@ -40,7 +38,7 @@ protected <K, V> Map<K, V> cache(String type) {
 
     // Hacky because I dont want to deal with implementing the entire Map interface
     protected static class MapLike<K, V> implements Map<K, V> {
-        private final BiFunction<K, Supplier<V>, V> lock = InternalUtils.cachePublic();
+        private final Cache<K, V> lock = Cache.create();
         @SuppressWarnings("unchecked")
         private final Function<K, V> get = Whitebox.getMethod(lock, "get", (Class<K>)Object.class);
 
@@ -51,15 +49,15 @@ public V get(Object key) {
         }
         @Override
         public V put(K key, V value) {
-            return lock.apply(key, () -> value);
+            return lock.computeIfAbsent(key, k -> value);
         }
         @Override
         public V putIfAbsent(K key, V value) {
-            return lock.apply(key, () -> value);
+            return lock.computeIfAbsent(key, k -> value);
         }
         @Override
         public V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) {
-            return lock.apply(key, () ->  mappingFunction.apply(key));
+            return lock.computeIfAbsent(key, mappingFunction::apply);
         }
         @Override public int size() { throw new UnsupportedOperationException(); }
         @Override public boolean isEmpty() { throw new UnsupportedOperationException(); }

src/main/java/module-info.java

@@ -14,6 +14,12 @@
 
     exports net.minecraftforge.eventbus;
     exports net.minecraftforge.eventbus.api;
+
+    /**
+     * Internal classes that may change or be removed at any time without notice
+     */
+    exports net.minecraftforge.eventbus.internal to net.minecraftforge.eventbus.test;
+
     provides cpw.mods.modlauncher.serviceapi.ILaunchPluginService with net.minecraftforge.eventbus.service.ModLauncherService;
     provides net.minecraftforge.eventbus.IEventBusEngine with net.minecraftforge.eventbus.EventBusEngine;
-}
+}

src/main/java/net/minecraftforge/eventbus/Cache.java

@@ -7,6 +7,8 @@
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
 import java.lang.reflect.Modifier;
+
+import net.minecraftforge.eventbus.internal.Cache;
 import org.objectweb.asm.ClassWriter;
 import org.objectweb.asm.MethodVisitor;
 import org.objectweb.asm.Type;
@@ -21,7 +23,7 @@ public class ClassLoaderFactory implements IEventListenerFactory {
     private static final String HANDLER_DESC = Type.getInternalName(IEventListener.class);
     private static final String HANDLER_FUNC_DESC = Type.getMethodDescriptor(Type.VOID_TYPE, Type.getType(Event.class));
     private static final ASMClassLoader LOADER = new ASMClassLoader();
-    private static final Cache<Method, Class<?>> cache = InternalUtils.cache();
+    private static final Cache<Method, Class<?>> cache = Cache.create();
 
     @Override
     public IEventListener create(Method method, Object target) throws InstantiationException, IllegalAccessException, IllegalArgumentException, InvocationTargetException, NoSuchMethodException, SecurityException, ClassNotFoundException {
@@ -34,14 +36,14 @@ public IEventListener create(Method method, Object target) throws InstantiationE
 
 
     protected Class<?> createWrapper(Method callback) throws ClassNotFoundException {
-        return cache.computeIfAbsent(callback, () -> {
+        return cache.computeIfAbsent(callback, k -> {
             var node = new ClassNode();
-            transformNode(getUniqueName(callback), callback, node);
+            transformNode(getUniqueName(k), k, node);
             return node;
         }, ClassLoaderFactory::defineClass);
     }
 
-    private static final Class<?> defineClass(ClassNode node) {
+    private static Class<?> defineClass(ClassNode node) {
         var cw = new ClassWriter(0);
         node.accept(cw);
         return LOADER.define(node.name.replace('/', '.'), cw.toByteArray());
@@ -111,7 +113,7 @@ else if (isInterface)
         target.visitEnd();
     }
 
-    private static class ASMClassLoader extends ClassLoader {
+    private static final class ASMClassLoader extends ClassLoader {
         private ASMClassLoader() {
             super(null);
         }

src/main/java/net/minecraftforge/eventbus/ClassLoaderFactory.java

@@ -7,6 +7,8 @@
 import org.apache.logging.log4j.Marker;
 import org.apache.logging.log4j.MarkerManager;
 
-class LogMarkers {
+final class LogMarkers {
+    private LogMarkers() {}
+
     static final Marker EVENTBUS = MarkerManager.getMarker("EVENTBUS");
 }

src/main/java/net/minecraftforge/eventbus/LogMarkers.java

@@ -5,15 +5,16 @@
 package net.minecraftforge.eventbus;
 
 import java.lang.reflect.Method;
-import java.util.HashMap;
 import java.util.Optional;
+
+import net.minecraftforge.eventbus.internal.Cache;
 import org.objectweb.asm.tree.ClassNode;
 
 import cpw.mods.modlauncher.Launcher;
 import cpw.mods.modlauncher.api.IModuleLayerManager;
 
 public class ModLauncherFactory extends ClassLoaderFactory {
-    private static final Cache<String, Method> PENDING = InternalUtils.cache();
+    private static final Cache<String, Method> PENDING = Cache.create();
     private Optional<ClassLoader> gameClassLoader = null;
 
     @Override
@@ -25,7 +26,7 @@ protected Class<?> createWrapper(Method callback) throws ClassNotFoundException
 
     private void enqueueWrapper(Method callback) {
         String name = getUniqueName(callback);
-        PENDING.computeIfAbsent(name, () -> callback);
+        PENDING.computeIfAbsent(name, k -> callback);
     }
 
     public static boolean hasPendingWrapperClass(final String className) {

src/main/java/net/minecraftforge/eventbus/ModLauncherFactory.java

@@ -8,7 +8,9 @@
 
 import org.objectweb.asm.tree.MethodNode;
 
-class Names {
+final class Names {
+    private Names() {}
+
     static final String SUBSCRIBE_EVENT = "Lnet/minecraftforge/eventbus/api/SubscribeEvent;";
     static final String HAS_RESULT = "Lnet/minecraftforge/eventbus/api/Event$HasResult;";
     static final Method HAS_RESULT_M = new Method("hasResult", getMethodDescriptor(BOOLEAN_TYPE));
@@ -26,7 +28,7 @@ class Names {
     static final Method INIT_M = new Method("<init>", getMethodDescriptor(VOID_TYPE));
     static final Method STATIC_INIT_M = new Method("<clinit>", getMethodDescriptor(VOID_TYPE));
 
-    static record Method(String name, String desc) {
+    record Method(String name, String desc) {
         boolean equals(MethodNode node) {
             return this.name.equals(node.name) && this.desc.equals(node.desc);
         }

src/main/java/net/minecraftforge/eventbus/Names.java

@@ -4,22 +4,21 @@
  */
 package net.minecraftforge.eventbus.api;
 
-import net.minecraftforge.eventbus.InternalUtils;
 import net.minecraftforge.eventbus.ListenerList;
 import net.minecraftforge.eventbus.api.Event.HasResult;
+import net.minecraftforge.eventbus.internal.Cache;
 
 import java.lang.annotation.Annotation;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Modifier;
-import java.util.function.BiFunction;
-import java.util.function.Supplier;
 
 public class EventListenerHelper {
-    private static final BiFunction<Class<?>, Supplier<ListenerList>, ListenerList> listeners = InternalUtils.cachePublic();
+    private static final Cache<Class<?>, ListenerList> listeners = Cache.create();
     private static final ListenerList EVENTS_LIST = new ListenerList();
-    private static final BiFunction<Class<?>, Supplier<Boolean>, Boolean> cancelable = InternalUtils.cachePublic();
-    private static final BiFunction<Class<?>, Supplier<Boolean>, Boolean> hasResult = InternalUtils.cachePublic();
+    private static final Cache<Class<?>, Boolean> cancelable = Cache.create();
+    private static final Cache<Class<?>, Boolean> hasResult = Cache.create();
+
     /**
      * Returns a {@link ListenerList} object that contains all listeners
      * that are registered to this event class.
@@ -35,7 +34,13 @@ public static ListenerList getListenerList(Class<?> eventClass) {
 
     static ListenerList getListenerListInternal(Class<?> eventClass, boolean fromInstanceCall) {
         if (eventClass == Event.class) return EVENTS_LIST; // Small optimization, bypasses all the locks/maps.
-        return listeners.apply(eventClass, () -> computeListenerList(eventClass, fromInstanceCall));
+
+        // Attempt to get the ListenerList directly from the cache first. This avoids an allocating lambda on cache hit
+        var ret = listeners.get(eventClass);
+        if (ret != null) return ret;
+
+        // Cache miss, check again (for thread-safety) and compute the ListenerList if still absent
+        return listeners.computeIfAbsent(eventClass, k -> computeListenerList(k, fromInstanceCall));
     }
 
     private static ListenerList computeListenerList(Class<?> eventClass, boolean fromInstanceCall) {
@@ -66,14 +71,17 @@ static boolean hasResult(Class<?> eventClass) {
         return hasAnnotation(eventClass, HasResult.class, hasResult);
     }
 
-    private static boolean hasAnnotation(Class<?> eventClass, Class<? extends Annotation> annotation, BiFunction<Class<?>, Supplier<Boolean>, Boolean> cache) {
+    private static boolean hasAnnotation(Class<?> eventClass, Class<? extends Annotation> annotation, Cache<Class<?>, Boolean> cache) {
         if (eventClass == Event.class || eventClass == Object.class)
             return false;
 
-        return cache.apply(eventClass, () -> {
-            if (eventClass.isAnnotationPresent(annotation))
+        var ret = cache.get(eventClass);
+        if (ret != null) return ret;
+
+        return cache.computeIfAbsent(eventClass, k -> {
+            if (k.isAnnotationPresent(annotation))
                 return true;
-            var parent = eventClass.getSuperclass();
+            var parent = k.getSuperclass();
             return parent != null && hasAnnotation(parent, annotation, cache);
         });
     }

src/main/java/net/minecraftforge/eventbus/api/EventListenerHelper.java

@@ -2,23 +2,24 @@
  * Copyright (c) Forge Development LLC
  * SPDX-License-Identifier: LGPL-2.1-only
  */
-package net.minecraftforge.eventbus;
+package net.minecraftforge.eventbus.internal;
 
-import java.util.HashMap;
-import java.util.function.BiFunction;
-import java.util.function.Supplier;
 import org.jetbrains.annotations.ApiStatus;
 
-/* Internal Utility class, only public so that other classes in the EventBus project can access things.
- * MODDERS SHOULD NEVER TOUCH THIS. And I reserve the right to do any breaking changes I want to this class.
- */
+import java.util.HashMap;
+import java.util.function.Function;
+
 @ApiStatus.Internal
-public class InternalUtils {
-    public static <K,V> BiFunction<K, Supplier<V>, V> cachePublic() {
-        return cache();
+public interface Cache<K, V> {
+    V get(K key);
+
+    default V computeIfAbsent(K key, Function<K, V> factory) {
+        return computeIfAbsent(key, factory, Function.identity());
     }
 
-    static <K, V> Cache<K, V> cache() {
+    <I> V computeIfAbsent(K key, Function<K, I> factory, Function<I, V> finalizer);
+
+    static <K, V> Cache<K, V> create() {
         var type = System.getProperty("eb.cache_type");
         if (type == null || "concurrent".equals(type))
             return new CacheConcurrent<>();

…necraftforge/eventbus/InternalUtils.java …ecraftforge/eventbus/internal/Cache.javasrc/main/java/net/minecraftforge/eventbus/InternalUtils.java renamed to src/main/java/net/minecraftforge/eventbus/internal/Cache.java src/main/java/net/minecraftforge/eventbus/InternalUtils.java renamed to src/main/java/net/minecraftforge/eventbus/internal/Cache.java

@@ -2,24 +2,19 @@
  * Copyright (c) Forge Development LLC
  * SPDX-License-Identifier: LGPL-2.1-only
  */
-package net.minecraftforge.eventbus;
+package net.minecraftforge.eventbus.internal;
 
-import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.Function;
-import java.util.function.Supplier;
 
 /*
  * An implementation of the Cache class that uses a ConcurrentHashMap for the backing map.
  * This allows us to use no locks when calling the get method.
  * However, it has re-entrant issues when writing. So we guard that using a synchronized block
  */
-class CacheConcurrent<K,V> implements Cache<K, V> {
-    private Object lock = new Object();
-    private final Map<K, V> map;
-
+record CacheConcurrent<K, V>(ConcurrentHashMap<K, V> map, Object lock) implements Cache<K, V> {
     CacheConcurrent() {
-        this.map = new ConcurrentHashMap<>(32);
+        this(new ConcurrentHashMap<>(32), new Object());
     }
 
     @Override
@@ -28,7 +23,7 @@ public V get(K key) {
     }
 
     @Override
-    public <I> V computeIfAbsent(K key, Supplier<I> factory, Function<I, V> finalizer) {
+    public <I> V computeIfAbsent(K key, Function<K, I> factory, Function<I, V> finalizer) {
         // This is a put once map, so lets try checking if the map has this value.
         // Should be thread safe to read without lock as any writes will be guarded
         var ret = get(key);
@@ -39,7 +34,7 @@ public <I> V computeIfAbsent(K key, Supplier<I> factory, Function<I, V> finalize
 
         // Let's pre-compute our new value. This could take a while, as well as recursively call this
         // function. as such, we need to make sure we don't hold a lock when we do this
-        var intermediate = factory.get();
+        var intermediate = factory.apply(key);
 
         // We are actually gunna modify the map now, so prevent other threads form doing so
         synchronized (lock) {