From c8ec6ab67d3afa23c29637f75122b90f4b6bf131 Mon Sep 17 00:00:00 2001 From: apple502j <33279053+apple502j@users.noreply.github.com> Date: Thu, 27 Jun 2024 01:45:53 +0900 Subject: [PATCH 1/5] Add Event#hasListener --- .../java/net/fabricmc/fabric/api/event/Event.java | 11 +++++++++++ .../fabric/impl/base/event/ArrayBackedEvent.java | 5 +++++ 2 files changed, 16 insertions(+) diff --git a/fabric-api-base/src/main/java/net/fabricmc/fabric/api/event/Event.java b/fabric-api-base/src/main/java/net/fabricmc/fabric/api/event/Event.java index db36fe4aa3..e763760147 100644 --- a/fabric-api-base/src/main/java/net/fabricmc/fabric/api/event/Event.java +++ b/fabric-api-base/src/main/java/net/fabricmc/fabric/api/event/Event.java @@ -88,4 +88,15 @@ public void register(Identifier phase, T listener) { public void addPhaseOrdering(Identifier firstPhase, Identifier secondPhase) { // This is not abstract to avoid breaking existing Event subclasses, but they should really not be subclassing Event. } + + /** + * Returns whether the event has any listeners. Useful for skipping steps related to + * event invocation, such as making context objects, when the event has no listeners. + * This method is unnecessary when the only code guarded by this check is the event invocation. + * + * @return whether the event has any listeners + */ + public boolean hasListener() { + return true; + } } diff --git a/fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/ArrayBackedEvent.java b/fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/ArrayBackedEvent.java index d8d8699cda..3aeb2ac304 100644 --- a/fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/ArrayBackedEvent.java +++ b/fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/ArrayBackedEvent.java @@ -123,4 +123,9 @@ public void addPhaseOrdering(Identifier firstPhase, Identifier secondPhase) { rebuildInvoker(handlers.length); } } + + @Override + public boolean hasListener() { + return this.handlers.length > 0; + } } From 344662e6b77febf30f9cee6a1ee9d69e075db364 Mon Sep 17 00:00:00 2001 From: apple502j <33279053+apple502j@users.noreply.github.com> Date: Thu, 27 Jun 2024 01:49:22 +0900 Subject: [PATCH 2/5] Remove functionality of EventFactory#invalidate --- .../fabricmc/fabric/api/event/EventFactory.java | 5 +---- .../fabric/impl/base/event/ArrayBackedEvent.java | 2 +- .../fabric/impl/base/event/EventFactoryImpl.java | 15 +-------------- 3 files changed, 3 insertions(+), 19 deletions(-) diff --git a/fabric-api-base/src/main/java/net/fabricmc/fabric/api/event/EventFactory.java b/fabric-api-base/src/main/java/net/fabricmc/fabric/api/event/EventFactory.java index 76b515ef9f..7a164b9987 100644 --- a/fabric-api-base/src/main/java/net/fabricmc/fabric/api/event/EventFactory.java +++ b/fabric-api-base/src/main/java/net/fabricmc/fabric/api/event/EventFactory.java @@ -126,14 +126,11 @@ public static boolean isProfilingEnabled() { } /** - * Invalidate and re-create all existing "invoker" instances across - * events created by this EventFactory. Use this if, for instance, - * the profilingEnabled field changes. + * Does nothing. * * @deprecated Do not use, will be removed in a future release. */ @Deprecated(forRemoval = true) public static void invalidate() { - EventFactoryImpl.invalidate(); } } diff --git a/fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/ArrayBackedEvent.java b/fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/ArrayBackedEvent.java index 3aeb2ac304..004d80e6cc 100644 --- a/fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/ArrayBackedEvent.java +++ b/fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/ArrayBackedEvent.java @@ -50,7 +50,7 @@ class ArrayBackedEvent extends Event { update(); } - void update() { + private void update() { this.invoker = invokerFactory.apply(handlers); } diff --git a/fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/EventFactoryImpl.java b/fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/EventFactoryImpl.java index bbf357d5f2..6dd7037f2e 100644 --- a/fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/EventFactoryImpl.java +++ b/fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/EventFactoryImpl.java @@ -23,30 +23,17 @@ import java.lang.reflect.Method; import java.lang.reflect.Modifier; import java.lang.reflect.Proxy; -import java.util.Collections; -import java.util.Set; import java.util.function.Function; -import com.google.common.collect.MapMaker; - import net.minecraft.util.Identifier; import net.fabricmc.fabric.api.event.Event; public final class EventFactoryImpl { - private static final Set> ARRAY_BACKED_EVENTS - = Collections.newSetFromMap(new MapMaker().weakKeys().makeMap()); - private EventFactoryImpl() { } - public static void invalidate() { - ARRAY_BACKED_EVENTS.forEach(ArrayBackedEvent::update); - } - public static Event createArrayBacked(Class type, Function invokerFactory) { - ArrayBackedEvent event = new ArrayBackedEvent<>(type, invokerFactory); - ARRAY_BACKED_EVENTS.add(event); - return event; + return new ArrayBackedEvent<>(type, invokerFactory); } public static void ensureContainsDefault(Identifier[] defaultPhases) { From 734a3bbcb00f762ca09c83432a7f5c98e9c54d30 Mon Sep 17 00:00:00 2001 From: apple502j <33279053+apple502j@users.noreply.github.com> Date: Thu, 27 Jun 2024 01:50:00 +0900 Subject: [PATCH 3/5] Remove unused code Although this is cool, there is no reason to keep this as part of shipped code. --- .../impl/base/event/EventFactoryImpl.java | 61 ------------------- 1 file changed, 61 deletions(-) diff --git a/fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/EventFactoryImpl.java b/fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/EventFactoryImpl.java index 6dd7037f2e..ead49d3de2 100644 --- a/fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/EventFactoryImpl.java +++ b/fabric-api-base/src/main/java/net/fabricmc/fabric/impl/base/event/EventFactoryImpl.java @@ -16,13 +16,6 @@ package net.fabricmc.fabric.impl.base.event; -import java.lang.invoke.MethodHandle; -import java.lang.invoke.MethodHandles; -import java.lang.invoke.MethodType; -import java.lang.reflect.Array; -import java.lang.reflect.Method; -import java.lang.reflect.Modifier; -import java.lang.reflect.Proxy; import java.util.function.Function; import net.minecraft.util.Identifier; @@ -55,58 +48,4 @@ public static void ensureNoDuplicates(Identifier[] defaultPhases) { } } } - - // Code originally by sfPlayer1. - // Unfortunately, it's slightly slower than just passing an empty array in the first place. - private static T buildEmptyInvoker(Class handlerClass, Function invokerSetup) { - // find the functional interface method - Method funcIfMethod = null; - - for (Method m : handlerClass.getMethods()) { - if ((m.getModifiers() & (Modifier.STRICT | Modifier.PRIVATE)) == 0) { - if (funcIfMethod != null) { - throw new IllegalStateException("Multiple virtual methods in " + handlerClass + "; cannot build empty invoker!"); - } - - funcIfMethod = m; - } - } - - if (funcIfMethod == null) { - throw new IllegalStateException("No virtual methods in " + handlerClass + "; cannot build empty invoker!"); - } - - Object defValue = null; - - try { - // concert to mh, determine its type without the "this" reference - MethodHandle target = MethodHandles.lookup().unreflect(funcIfMethod); - MethodType type = target.type().dropParameterTypes(0, 1); - - if (type.returnType() != void.class) { - // determine default return value by invoking invokerSetup.apply(T[0]) with all-jvm-default args (null for refs, false for boolean, etc.) - // explicitCastArguments is being used to cast Object=null to the jvm default value for the correct type - - // construct method desc (TLjava/lang/Object;Ljava/lang/Object;...)R where T = invoker ref ("this"), R = invoker ret type and args 1+ are Object for each non-"this" invoker arg - MethodType objTargetType = MethodType.genericMethodType(type.parameterCount()).changeReturnType(type.returnType()).insertParameterTypes(0, target.type().parameterType(0)); - // explicit cast to translate to the invoker args from Object to their real type, inferring jvm default values - MethodHandle objTarget = MethodHandles.explicitCastArguments(target, objTargetType); - - // build invocation args with 0 = "this", 1+ = null - Object[] args = new Object[target.type().parameterCount()]; - //noinspection unchecked - args[0] = invokerSetup.apply((T[]) Array.newInstance(handlerClass, 0)); - - // retrieve default by invoking invokerSetup.apply(T[0]).targetName(def,def,...) - defValue = objTarget.invokeWithArguments(args); - } - } catch (Throwable t) { - throw new RuntimeException(t); - } - - final Object returnValue = defValue; - //noinspection unchecked - return (T) Proxy.newProxyInstance(EventFactoryImpl.class.getClassLoader(), new Class[]{handlerClass}, - (proxy, method, args) -> returnValue); - } } From 65b836582a997ca07c9f57bac3091ab91171f5eb Mon Sep 17 00:00:00 2001 From: apple502j <33279053+apple502j@users.noreply.github.com> Date: Sat, 6 Jul 2024 11:40:19 +0900 Subject: [PATCH 4/5] Add tests and move them to junit --- .../fabricmc/fabric/test/base/EventTests.java | 100 +++++++++--------- .../test/base/FabricApiBaseTestInit.java | 2 - 2 files changed, 50 insertions(+), 52 deletions(-) rename fabric-api-base/src/{testmod => test}/java/net/fabricmc/fabric/test/base/EventTests.java (75%) diff --git a/fabric-api-base/src/testmod/java/net/fabricmc/fabric/test/base/EventTests.java b/fabric-api-base/src/test/java/net/fabricmc/fabric/test/base/EventTests.java similarity index 75% rename from fabric-api-base/src/testmod/java/net/fabricmc/fabric/test/base/EventTests.java rename to fabric-api-base/src/test/java/net/fabricmc/fabric/test/base/EventTests.java index 5c2df49a45..5b8909d78f 100644 --- a/fabric-api-base/src/testmod/java/net/fabricmc/fabric/test/base/EventTests.java +++ b/fabric-api-base/src/test/java/net/fabricmc/fabric/test/base/EventTests.java @@ -16,15 +16,20 @@ package net.fabricmc.fabric.test.base; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + import java.util.ArrayList; import java.util.List; -import java.util.Objects; import java.util.function.Consumer; import java.util.function.Function; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import net.minecraft.Bootstrap; +import net.minecraft.SharedConstants; import net.minecraft.util.Identifier; import net.fabricmc.fabric.api.event.Event; @@ -32,47 +37,38 @@ import net.fabricmc.fabric.impl.base.toposort.NodeSorting; public class EventTests { - private static final Logger LOGGER = LoggerFactory.getLogger("fabric-api-base"); - - public static void run() { - long time1 = System.currentTimeMillis(); - - testDefaultPhaseOnly(); - testMultipleDefaultPhases(); - testAddedPhases(); - testCycle(); - NodeSorting.ENABLE_CYCLE_WARNING = false; - testDeterministicOrdering(); - testTwoCycles(); - NodeSorting.ENABLE_CYCLE_WARNING = true; - - long time2 = System.currentTimeMillis(); - LOGGER.info("Event unit tests succeeded in {} milliseconds.", time2 - time1); + @BeforeAll + static void beforeAll() { + SharedConstants.createGameVersion(); + Bootstrap.initialize(); } - private static final Function INVOKER_FACTORY = listeners -> () -> { - for (Test test : listeners) { + private static final Function INVOKER_FACTORY = listeners -> () -> { + for (TestCallback test : listeners) { test.onTest(); } }; private static int currentListener = 0; - private static Event createEvent() { - return EventFactory.createArrayBacked(Test.class, INVOKER_FACTORY); + private static Event createEvent() { + return EventFactory.createArrayBacked(TestCallback.class, INVOKER_FACTORY); } - private static Test ensureOrder(int order) { + private static TestCallback ensureOrder(int order) { return () -> { assertEquals(order, currentListener); ++currentListener; }; } - private static void testDefaultPhaseOnly() { - Event event = createEvent(); + @Test + public void testDefaultPhaseOnly() { + Event event = createEvent(); + assertFalse(event.hasListener(), "Newly created event does not have listeners"); event.register(ensureOrder(0)); + assertTrue(event.hasListener(), "hasListener returns true when event has a listener"); event.register(Event.DEFAULT_PHASE, ensureOrder(1)); event.register(ensureOrder(2)); @@ -81,12 +77,14 @@ private static void testDefaultPhaseOnly() { currentListener = 0; } - private static void testMultipleDefaultPhases() { + @Test + public void testMultipleDefaultPhases() { Identifier first = Identifier.of("fabric", "first"); Identifier second = Identifier.of("fabric", "second"); - Event event = EventFactory.createWithPhases(Test.class, INVOKER_FACTORY, first, second, Event.DEFAULT_PHASE); + Event event = EventFactory.createWithPhases(TestCallback.class, INVOKER_FACTORY, first, second, Event.DEFAULT_PHASE); event.register(second, ensureOrder(1)); + assertTrue(event.hasListener(), "hasListener returns true when event has a listener in non-default phases"); event.register(ensureOrder(2)); event.register(first, ensureOrder(0)); @@ -97,8 +95,9 @@ private static void testMultipleDefaultPhases() { } } - private static void testAddedPhases() { - Event event = createEvent(); + @Test + public void testAddedPhases() { + Event event = createEvent(); Identifier veryEarly = Identifier.of("fabric", "very_early"); Identifier early = Identifier.of("fabric", "early"); @@ -128,8 +127,9 @@ private static void testAddedPhases() { } } - private static void testCycle() { - Event event = createEvent(); + @Test + public void testCycle() { + Event event = createEvent(); Identifier a = Identifier.of("fabric", "a"); Identifier b1 = Identifier.of("fabric", "b1"); @@ -183,7 +183,9 @@ private static void testCycle() { * Notice the cycle z -> b -> y -> z. The elements of the cycle are ordered [b, y, z], and the cycle itself is ordered with its lowest id "b". * We get for the final order: [a, d, e, cycle [b, y, z], f]. */ - private static void testDeterministicOrdering() { + @Test + public void testDeterministicOrdering() { + NodeSorting.ENABLE_CYCLE_WARNING = false; Identifier a = Identifier.of("fabric", "a"); Identifier b = Identifier.of("fabric", "b"); Identifier d = Identifier.of("fabric", "d"); @@ -192,7 +194,7 @@ private static void testDeterministicOrdering() { Identifier y = Identifier.of("fabric", "y"); Identifier z = Identifier.of("fabric", "z"); - List>> dependencies = List.of( + List>> dependencies = List.of( ev -> ev.addPhaseOrdering(a, z), ev -> ev.addPhaseOrdering(d, e), ev -> ev.addPhaseOrdering(e, z), @@ -202,9 +204,9 @@ private static void testDeterministicOrdering() { ); testAllPermutations(new ArrayList<>(), dependencies, selectedDependencies -> { - Event event = createEvent(); + Event event = createEvent(); - for (Consumer> dependency : selectedDependencies) { + for (Consumer> dependency : selectedDependencies) { dependency.accept(event); } @@ -220,22 +222,25 @@ private static void testDeterministicOrdering() { assertEquals(7, currentListener); currentListener = 0; }); + NodeSorting.ENABLE_CYCLE_WARNING = true; } /** - * Test deterministic phase sorting with two cycles. + * TestCallback deterministic phase sorting with two cycles. * 
 	 * e --> a <--> b <-- d <--> c
 	 *
*/ - private static void testTwoCycles() { + @Test + public void testTwoCycles() { + NodeSorting.ENABLE_CYCLE_WARNING = false; Identifier a = Identifier.of("fabric", "a"); Identifier b = Identifier.of("fabric", "b"); Identifier c = Identifier.of("fabric", "c"); Identifier d = Identifier.of("fabric", "d"); Identifier e = Identifier.of("fabric", "e"); - List>> dependencies = List.of( + List>> dependencies = List.of( ev -> ev.addPhaseOrdering(e, a), ev -> ev.addPhaseOrdering(a, b), ev -> ev.addPhaseOrdering(b, a), @@ -245,9 +250,9 @@ private static void testTwoCycles() { ); testAllPermutations(new ArrayList<>(), dependencies, selectedDependencies -> { - Event event = createEvent(); + Event event = createEvent(); - for (Consumer> dependency : selectedDependencies) { + for (Consumer> dependency : selectedDependencies) { dependency.accept(event); } @@ -261,11 +266,12 @@ private static void testTwoCycles() { assertEquals(5, currentListener); currentListener = 0; }); + NodeSorting.ENABLE_CYCLE_WARNING = true; } @SuppressWarnings("SuspiciousListRemoveInLoop") private static void testAllPermutations(List selected, List toSelect, Consumer> action) { - if (toSelect.size() == 0) { + if (toSelect.isEmpty()) { action.accept(selected); } else { for (int i = 0; i < toSelect.size(); ++i) { @@ -273,19 +279,13 @@ private static void testAllPermutations(List selected, List toSelect, List remaining = new ArrayList<>(toSelect); remaining.remove(i); testAllPermutations(selected, remaining, action); - selected.remove(selected.size()-1); + selected.removeLast(); } } } @FunctionalInterface - interface Test { + interface TestCallback { void onTest(); } - - private static void assertEquals(Object expected, Object actual) { - if (!Objects.equals(expected, actual)) { - throw new AssertionError(String.format("assertEquals failed%nexpected: %s%n but was: %s", expected, actual)); - } - } } diff --git a/fabric-api-base/src/testmod/java/net/fabricmc/fabric/test/base/FabricApiBaseTestInit.java b/fabric-api-base/src/testmod/java/net/fabricmc/fabric/test/base/FabricApiBaseTestInit.java index 269e0249b3..2f7e185b68 100644 --- a/fabric-api-base/src/testmod/java/net/fabricmc/fabric/test/base/FabricApiBaseTestInit.java +++ b/fabric-api-base/src/testmod/java/net/fabricmc/fabric/test/base/FabricApiBaseTestInit.java @@ -45,7 +45,5 @@ public void onInitialize() { return 1; })); }); - - EventTests.run(); } } From b7b23fddcaa28b524bcf268b6d9f687050f5a8c9 Mon Sep 17 00:00:00 2001 From: apple502j <33279053+apple502j@users.noreply.github.com> Date: Sun, 21 Jul 2024 03:27:26 +0900 Subject: [PATCH 5/5] Remove unnecessary BeforeAll --- .../java/net/fabricmc/fabric/test/base/EventTests.java | 9 --------- 1 file changed, 9 deletions(-) diff --git a/fabric-api-base/src/test/java/net/fabricmc/fabric/test/base/EventTests.java b/fabric-api-base/src/test/java/net/fabricmc/fabric/test/base/EventTests.java index 5b8909d78f..33cba30c90 100644 --- a/fabric-api-base/src/test/java/net/fabricmc/fabric/test/base/EventTests.java +++ b/fabric-api-base/src/test/java/net/fabricmc/fabric/test/base/EventTests.java @@ -25,11 +25,8 @@ import java.util.function.Consumer; import java.util.function.Function; -import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; -import net.minecraft.Bootstrap; -import net.minecraft.SharedConstants; import net.minecraft.util.Identifier; import net.fabricmc.fabric.api.event.Event; @@ -37,12 +34,6 @@ import net.fabricmc.fabric.impl.base.toposort.NodeSorting; public class EventTests { - @BeforeAll - static void beforeAll() { - SharedConstants.createGameVersion(); - Bootstrap.initialize(); - } - private static final Function INVOKER_FACTORY = listeners -> () -> { for (TestCallback test : listeners) { test.onTest();