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

Skip phase tracking notifiers at registration time rather than post time #66

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PaintNinja
Copy link
Contributor

This removes the need for additional checks per listener when posting.

@PaintNinja PaintNinja added the enhancement New feature or request label Jan 16, 2025
@Jonathing
Copy link
Member

I genuinely don't know how to test this. EventBus is still really foreign to me. But the code looks sane. I'll leave this to Lex.

@Jonathing Jonathing requested a review from LexManos January 23, 2025 16:24
@PaintNinja
Copy link
Contributor Author

PaintNinja commented Jan 23, 2025

TLDR the PR is marked as a draft with minimal explanation because I was trying to troubleshoot what turned out to be a JVM bug.

Allow me to elaborate what this PR does and how...

First off, EventBus has a feature called "phase tracking" - this is basically telling events what priority they're in, which is used to prevent cancelling during the monitor phase. It does this by adding extra listeners for each priority that run just before the user defined listeners.

Currently, the phase tracking notifiers are always registered, even if the EventBus they're associated with has phase tracking disabled. To effectively disable these notifiers, it does an if check and class comparison for every listener on every post.

This PR makes it so that if check is unnecessary as it instead lets the ListenerListInst know if phase tracking is wanted during registration time. It skips adding the phase tracking notifiers to the listener list entirely when it knows they're not wanted, rather than checking every listener at post time.

In theory this should provide a nice performance uplift to posting, as it decreases the array size returned by the getListeners() method, makes the loop more predictable to the JIT and reduces the amount of work to do on each iteration.

In practice, posting a single listener is faster with this PR but posting hundreds is counter-intuitively slower. I have absolutely no idea why and at this point I'm convinced it's a JVM bug.

To test this, run the JMH task with gradlew jmh -Pbench="BenchmarkModLauncher.Posting.Static.postStatic" both on the master branch and this PR branch. You can throw a println(listeners.length) in the EventBus#post method and disable phase tracking in ModLauncherBenchmarks/BenchmarkModLauncher by adding setPhaseTracking(false) in the BusBuilder call where the bus is created to see the number of listeners reduce vs when phase tracking is enabled.

@Jonathing
Copy link
Member

That's rough

@LexManos
Copy link
Member

I have a suspission it has to do with the field becoming package private and list.phaseTracking not being final.
Perhaps a better approach would be to change the ListenerList.resize function take in the eventbus/phaseTracking boolean. This was the check is done once at list creation and can be finalized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants