Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Event#hasListener, remove unused junk #3889

Open
wants to merge 5 commits into
base: 1.21
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is a bad pattern. If you are concerned about allocation, reuse the context object. In typical modpacks, most events will be used and these checks will be pointless anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats a fair point, @apple502j did you have a specific use case in mind?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A usecase I had for this was for a deprecated event, where actually properly servicing it required setting up some expensive stuff

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, not all context objects can be reused.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you link some specific examples?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/BasiqueEvangelist/ne-v-seti/blob/sanity-1.17/src/main/java/me/basiqueevangelist/nevseti/OfflineDataCache.java#L30
I guess I misremembered specifically how much it would take to service this event, but I think it will still be useful for events that are deprecated and require running extra logic to run their invokers

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not resolved, please don't mark it as resolved.

return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class ArrayBackedEvent<T> extends Event<T> {
update();
}

void update() {
private void update() {
this.invoker = invokerFactory.apply(handlers);
}

Expand Down Expand Up @@ -123,4 +123,9 @@ public void addPhaseOrdering(Identifier firstPhase, Identifier secondPhase) {
rebuildInvoker(handlers.length);
}
}

@Override
public boolean hasListener() {
return this.handlers.length > 0;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,37 +16,17 @@

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.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<ArrayBackedEvent<?>> ARRAY_BACKED_EVENTS
= Collections.newSetFromMap(new MapMaker().weakKeys().makeMap());

private EventFactoryImpl() { }

public static void invalidate() {
ARRAY_BACKED_EVENTS.forEach(ArrayBackedEvent::update);
}

public static <T> Event<T> createArrayBacked(Class<? super T> type, Function<T[], T> invokerFactory) {
ArrayBackedEvent<T> event = new ArrayBackedEvent<>(type, invokerFactory);
ARRAY_BACKED_EVENTS.add(event);
return event;
return new ArrayBackedEvent<>(type, invokerFactory);
}

public static void ensureContainsDefault(Identifier[] defaultPhases) {
Expand All @@ -68,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> T buildEmptyInvoker(Class<T> handlerClass, Function<T[], T> 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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,16 @@

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.Test;

import net.minecraft.util.Identifier;

Expand All @@ -32,47 +34,32 @@
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);
}

private static final Function<Test[], Test> INVOKER_FACTORY = listeners -> () -> {
for (Test test : listeners) {
private static final Function<TestCallback[], TestCallback> INVOKER_FACTORY = listeners -> () -> {
for (TestCallback test : listeners) {
test.onTest();
}
};

private static int currentListener = 0;

private static Event<Test> createEvent() {
return EventFactory.createArrayBacked(Test.class, INVOKER_FACTORY);
private static Event<TestCallback> 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<Test> event = createEvent();
@Test
public void testDefaultPhaseOnly() {
Event<TestCallback> 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));

Expand All @@ -81,12 +68,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<Test> event = EventFactory.createWithPhases(Test.class, INVOKER_FACTORY, first, second, Event.DEFAULT_PHASE);
Event<TestCallback> 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));

Expand All @@ -97,8 +86,9 @@ private static void testMultipleDefaultPhases() {
}
}

private static void testAddedPhases() {
Event<Test> event = createEvent();
@Test
public void testAddedPhases() {
Event<TestCallback> event = createEvent();

Identifier veryEarly = Identifier.of("fabric", "very_early");
Identifier early = Identifier.of("fabric", "early");
Expand Down Expand Up @@ -128,8 +118,9 @@ private static void testAddedPhases() {
}
}

private static void testCycle() {
Event<Test> event = createEvent();
@Test
public void testCycle() {
Event<TestCallback> event = createEvent();

Identifier a = Identifier.of("fabric", "a");
Identifier b1 = Identifier.of("fabric", "b1");
Expand Down Expand Up @@ -183,7 +174,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");
Expand All @@ -192,7 +185,7 @@ private static void testDeterministicOrdering() {
Identifier y = Identifier.of("fabric", "y");
Identifier z = Identifier.of("fabric", "z");

List<Consumer<Event<Test>>> dependencies = List.of(
List<Consumer<Event<TestCallback>>> dependencies = List.of(
ev -> ev.addPhaseOrdering(a, z),
ev -> ev.addPhaseOrdering(d, e),
ev -> ev.addPhaseOrdering(e, z),
Expand All @@ -202,9 +195,9 @@ private static void testDeterministicOrdering() {
);

testAllPermutations(new ArrayList<>(), dependencies, selectedDependencies -> {
Event<Test> event = createEvent();
Event<TestCallback> event = createEvent();

for (Consumer<Event<Test>> dependency : selectedDependencies) {
for (Consumer<Event<TestCallback>> dependency : selectedDependencies) {
dependency.accept(event);
}

Expand All @@ -220,22 +213,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.
* <pre>
* e --> a <--> b <-- d <--> c
* </pre>
*/
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<Consumer<Event<Test>>> dependencies = List.of(
List<Consumer<Event<TestCallback>>> dependencies = List.of(
ev -> ev.addPhaseOrdering(e, a),
ev -> ev.addPhaseOrdering(a, b),
ev -> ev.addPhaseOrdering(b, a),
Expand All @@ -245,9 +241,9 @@ private static void testTwoCycles() {
);

testAllPermutations(new ArrayList<>(), dependencies, selectedDependencies -> {
Event<Test> event = createEvent();
Event<TestCallback> event = createEvent();

for (Consumer<Event<Test>> dependency : selectedDependencies) {
for (Consumer<Event<TestCallback>> dependency : selectedDependencies) {
dependency.accept(event);
}

Expand All @@ -261,31 +257,26 @@ private static void testTwoCycles() {
assertEquals(5, currentListener);
currentListener = 0;
});
NodeSorting.ENABLE_CYCLE_WARNING = true;
}

@SuppressWarnings("SuspiciousListRemoveInLoop")
private static <T> void testAllPermutations(List<T> selected, List<T> toSelect, Consumer<List<T>> action) {
if (toSelect.size() == 0) {
if (toSelect.isEmpty()) {
action.accept(selected);
} else {
for (int i = 0; i < toSelect.size(); ++i) {
selected.add(toSelect.get(i));
List<T> 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));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,5 @@ public void onInitialize() {
return 1;
}));
});

EventTests.run();
}
}
Loading