How can I make this faster?

Question

I have made an event system, however, it is too slow.

The issue, is that there are multiple entries in a map that I never actually added. I don't understand how they get there.

public class OrdinalMap<V> {

    private final Map<Integer, V> map;

    public OrdinalMap(Class<? extends Enum<?>> valueType, V virginValue) {
        map = new HashMap<Integer, V>();
        Enum<?>[] enums = valueType.getEnumConstants();
        for (int i = 0; i < enums.length; i++) {
            put(enums[i].ordinal(), virginValue);
        }
    }

    public OrdinalMap(Class<? extends Enum<?>> valueType) {
        this(valueType, null);
    }

    public V put(Integer key, V value) {
        return map.put(key, value);
    }

    public V get(Object o) {
        return map.get(o);
    }

    public Set<Entry<Integer, V>> entrySet() {
        return map.entrySet();
    }

}

I want to make dispatchEvent faster (less iterations). It has too many iterations because of registerListener

There are event handler methods inside of all of the other priorities, when they shouldn't be there. I can't figure out why there are there, but I'm certain it's in registerListener. Because they are inside all priorities, I have to use this check: if (mapping.getKey().getAnnotation(EventHandler.class).priority().ordinal() == entry.getKey()) {

Which makes it even slower.

@Override
public void dispatchEvent(Event event) {
    OrdinalMap<Map<Method, EventListener>> priorityMap = getRegistry().get(event.getClass());

    if (priorityMap != null) {
        CancellableEvent cancellableEvent = null;
        boolean cancellable;
        if (cancellable = event instanceof CancellableEvent) {
            cancellableEvent = (CancellableEvent) event;
            if (cancellableEvent.isCancelled()) return;
        }

        try {
            for (Entry<Integer, Map<Method, EventListener>> entry : priorityMap.entrySet()) {
                for (Entry<Method, EventListener> mapping : entry.getValue().entrySet()) {
                    if (mapping.getKey().getAnnotation(EventHandler.class).priority().ordinal() == entry.getKey()) {
                        mapping.getKey().invoke(mapping.getValue(), event);
                        if (cancellable && cancellableEvent.isCancelled()) return;
                    }
                }
            }
        } catch (InvocationTargetException | IllegalAccessException e) {
            e.printStackTrace();
        }
    }
}

@Override
public void registerListener(EventListener listener) {
    for (Method method : listener.getClass().getMethods()) {
        EventHandler handler = method.getAnnotation(EventHandler.class);
        if (handler != null) {
            Class<?>[] parameters = method.getParameterTypes();
            if (parameters.length == 1) {
                @SuppressWarnings("unchecked")
                Class<? extends Event> event = (Class<? extends Event>) parameters[0];
                EventPriority priority = handler.priority();

                OrdinalMap<Map<Method, EventListener>> priorityMap = getRegistry().get(event);
                if (priorityMap == null) {
                    priorityMap = new OrdinalMap<Map<Method, EventListener>>(EventPriority.class, (Map<Method, EventListener>) new HashMap<Method, EventListener>());
                }

                Map<Method, EventListener> methodMap = priorityMap.get(priority.ordinal());

                methodMap.put(method, listener);
                priorityMap.put(priority.ordinal(), methodMap);

                getRegistry().put(event, priorityMap);
            }
        }
    }
}

Answer 1

You are using maps so consider using thiere benefits instead of iterating on all entries

if (mapping.getKey().getAnnotation(EventHandler.class).priority().ordinal() == entry.getKey()) {

comparing two hahmap keys to find a match is not really a good idea.

how about the following, i hope i didn't make any thinking mistake

Set<Integer> priorityMapKeySet = priorityMap.keySet();
for (Map<Method, EventListener> mapping : priorityMap.values()) {
    if (priorityMapKeySet.contains(mapping.getKey().getAnnotation(EventHandler.class).priority().ordinal())) {
        mapping.getKey().invoke(mapping.getValue(), event);
        if (cancellable && cancellableEvent.isCancelled()) return;
    }
}

here you no longer have the outer for loop

My bad, didn't pay attention enough ...

But the idea is the same, when using hashmaps / hashsets one should always try to use get / contains instead of iterating, and for that one needs to design the registry the way that makes this prossible

Would the following work for your needs ? (untested)

private final static class Registry {

    private final static Map<String, Set<Integer>>  prioritySetByEventMap = new HashMap<>();
    private final static Map<String, EventListener> eventListenerByEventAndPriorityMap = new HashMap<>();
    private final static Map<String, Method> methodByEventAndListenerMap = new HashMap<>();

    public static Set<Integer> getPrioritySetByEvent(Class<Event> event) {
        return prioritySetByEventMap.get(event.getName());
    }

    public static synchronized void registerEventByPriority(Class<Event> event, Integer priority) {
        Set<Integer> ps = prioritySetByEventMap.get(event.getName());
        if(ps == null) {
            ps = new HashSet<>();
            prioritySetByEventMap.put(event.getName(), ps);
        }
        ps.add(priority);
    }

    public static EventListener getEventListenerByEventAndPriority(Class<Event> event, Integer priority) {
        String key = event.getName() + "-" + priority;
        return eventListenerByEventAndPriorityMap.get(key);
    }

    public static synchronized void registerEventListenerByEventAndPriority(Class<Event> event, Integer priority, EventListener listener) {
        String key = event.getName() + "-" + priority;
        eventListenerByEventAndPriorityMap.put(key, listener);
    }

    public static Method getMethodByEventAndListener(Class<Event> event, EventListener listener) {
        String key = listener.toString() + "-" + event.getName();
        return methodByEventAndListenerMap.get(key);
    }

    public static synchronized void registerMethodByEventAndListener(Class<Event> event, EventListener listener, Method method) {
        String key = listener.toString() + "-" + event.getName();
        methodByEventAndListenerMap.put(key, method);
    }
}

and

public void registerListener(EventListener listener) {
    for (Method method : listener.getClass().getMethods()) {
        EventHandler handler = method.getAnnotation(EventHandler.class);
        if (handler != null) {
            Class<?>[] parameters = method.getParameterTypes();
            if (parameters.length == 1) {

                Class<Event> event = (Class<Event>) parameters[0];

                EventPriority priority = handler.priority();

                Registry.registerEventByPriority(event, priority.ordinal());

                Registry.registerEventListenerByEventAndPriority(event, priority.ordinal(), listener);

                Registry.registerMethodByEventAndListener(event, listener, method);

            }
        }
    }
}


public void dispatchEvent(Event event) {
    Set<Integer> prioritySet = Registry.getPrioritySetByEvent((Class<Event>) event.getClass());

    if (prioritySet != null) {
        CancellableEvent cancellableEvent = null;
        boolean cancellable;
        if (cancellable = event instanceof CancellableEvent) {
            cancellableEvent = (CancellableEvent) event;
            if (cancellableEvent.isCancelled())
                return;
        }

        try {

            for(Integer priority : prioritySet) {

                EventListener listener = Registry.getEventListenerByEventAndPriority((Class<Event>) event.getClass(), priority);

                if(listener != null) {
                    Method m = Registry.getMethodByEventAndListener((Class<Event>) event.getClass(), listener);
                    if(m != null) {
                        m.invoke(listener, event);
                        if (cancellable && cancellableEvent.isCancelled()) {
                            return;
                        }
                    }
                }
            }

        } catch (InvocationTargetException | IllegalAccessException e) {
            e.printStackTrace();
        }
    }
}