Skip to content

Use a ConcurrentHashMap instead of a TreeMap but still sort via priority.#2514

Merged
Yesssssman merged 5 commits intoAntikythera-Studios:1.21.1from
Forixaim:1.21.1-EventRegistration-Fix
Apr 4, 2026
Merged

Use a ConcurrentHashMap instead of a TreeMap but still sort via priority.#2514
Yesssssman merged 5 commits intoAntikythera-Studios:1.21.1from
Forixaim:1.21.1-EventRegistration-Fix

Conversation

@Forixaim
Copy link
Copy Markdown
Collaborator

This is still a draft but feedback is still needed.

@Forixaim Forixaim requested a review from Yesssssman as a code owner March 30, 2026 21:24
@Yesssssman
Copy link
Copy Markdown
Collaborator

There are compile errors should be fixed according to this change in CancelableEventHook

@Yesssssman
Copy link
Copy Markdown
Collaborator

Thread safety requirement


The subscriber container should be thread-safe since NeoForge loads mods asynchronously, if event registration is called in the mod's constructor it will cause ConcurrentModificationException.

Tested by custom multi threading code:

public EpicFightMod(IEventBus modEventBus, ModContainer modContainer) {
        ...
        Thread t1 = new Thread(() -> {
            MutableInt mi = new MutableInt(0);

            for (int i = 0; i < 50; i++) {
                EpicFightEventHooks.Registry.ENTITY_PATCH.registerEvent(event -> {
                    System.out.println(mi.getAndIncrement() + "'th entity patch event");
                }, i);
            }
        });

        Thread t2 = new Thread(() -> {
            MutableInt mi = new MutableInt(1000);

            for (int i = 1000; i < 1050; i++) {
                EpicFightEventHooks.Registry.ENTITY_PATCH.registerEvent(event -> {
                    System.out.println(mi.getAndIncrement() + "'th entity patch event");
                }, i);
            }
        });

        Thread t3 = new Thread(() -> {
            MutableInt mi = new MutableInt(10000);

            for (int i = 10000; i < 10050; i++) {
                EpicFightEventHooks.Registry.ENTITY_PATCH.registerEvent(event -> {
                    System.out.println(mi.getAndIncrement() + "'th entity patch event");
                }, i);
            }
        });

        t3.start();
        t1.start();
        t2.start();
	}
Exception in thread "Thread-5" Exception in thread "Thread-4" java.util.ConcurrentModificationException
	at java.base/java.util.TreeMap.callMappingFunctionWithCheck(TreeMap.java:785)
	at java.base/java.util.TreeMap.computeIfAbsent(TreeMap.java:639)

Output:

10000'th entity patch event
10001'th entity patch event
10002'th entity patch event
10003'th entity patch event
1000'th entity patch event
1001'th entity patch event
1002'th entity patch event
1003'th entity patch event
1004'th entity patch event
1005'th entity patch event
1006'th entity patch event
1007'th entity patch event
1008'th entity patch event
1009'th entity patch event
1010'th entity patch event
1011'th entity patch event
1012'th entity patch event
1013'th entity patch event
1014'th entity patch event
1015'th entity patch event
1016'th entity patch event
1017'th entity patch event
1018'th entity patch event
1019'th entity patch event
1020'th entity patch event
1021'th entity patch event
1022'th entity patch event
1023'th entity patch event
1024'th entity patch event
1025'th entity patch event
1026'th entity patch event
1027'th entity patch event
1028'th entity patch event
1029'th entity patch event
1030'th entity patch event
1031'th entity patch event
0'th entity patch event
1'th entity patch event
2'th entity patch event
3'th entity patch event
4'th entity patch event
5'th entity patch event
6'th entity patch event
7'th entity patch event
8'th entity patch event

Due to the registration failing while running the thread, the event logs end in a random sequence.

Performance Optimization


Instead of sorting event priorities each time an event hook is called, we should pre-order the events so that the event hook just calls subscribers in order.

Tested code on PR (sort on trigger) vs local test code (sort on register)

public T post(T event) {
    long begin = System.currentTimeMillis();

    for (var subscriberList : this.subscribers.values()) {
        for (var subscriber : subscriberList) {
            if (subscriber.subscriptionType() instanceof DefaultEventSubscription<T> passiveSubscription) {
                passiveSubscription.fire(event);
            }
        }
    }

    long took = System.currentTimeMillis() - begin;
    System.out.println("ConcurrentSkipListMap + CopyOnWriteArrayList event call: " + (took));

    long begin2 = System.currentTimeMillis();
    List<EventListener<T>> buffer = Lists.newArrayList();

    this.subscribers.values().forEach(buffer::addAll);
    Comparator<EventListener<T>> eventListeners = Comparator.comparing(EventListener::priority);

    buffer.stream().sorted(eventListeners.reversed()).forEach(listener -> {
        if (listener.subscriptionType() instanceof DefaultEventSubscription<T> passiveSubscription) {
            passiveSubscription.fire(event);
        }
    });

    long took2 = System.currentTimeMillis() - begin2;

    System.out.println("concurrent map + arraylist event call: " + (took2));


    return event;
}

Output

ConcurrentSkipListMap + CopyOnWriteArrayList event call: 187
concurrent map + arraylist event call: 690

p.s. The sort order of event subscribers is descending

@Yesssssman Yesssssman merged commit bb5eeef into Antikythera-Studios:1.21.1 Apr 4, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants