[6.2] New aproach to optimizing cancelable events #99
Conversation
Jonathing
changed the title
This also prevents the lambdas from being wrapped multiple times when they need filtering which should help debug logging and performance.
There was a problem hiding this comment.
Being able to strip cancellation checks without always needing final/sealed is pretty nice.
This PR also bundles in some vaguely-related changes that influence performance, which when combined, causes regressions in some cases and improvements in others. I've left some braindump comments to help solve these regressions, but I'm on the fence on whether or not it'll be easier to split this up into multiple PRs.
Benchmarked with Adoptium JDK 17.0.14
Before:
Benchmark Mode Cnt Score Error Units
BenchmarkModLauncher.Posting.Dynamic.postDynamic avgt 5 6.268 ± 0.196 ns/op
BenchmarkModLauncher.Posting.Dynamic.postDynamicDozen avgt 5 78.684 ± 4.847 ns/op
BenchmarkModLauncher.Posting.Dynamic.postDynamicHundred avgt 5 917.504 ± 14.907 ns/op
BenchmarkModLauncher.Posting.Lambda.postLambda avgt 5 5.555 ± 0.118 ns/op
BenchmarkModLauncher.Posting.Lambda.postLambdaDozen avgt 5 74.029 ± 5.644 ns/op
BenchmarkModLauncher.Posting.Lambda.postLambdaHundred avgt 5 969.589 ± 86.049 ns/op
BenchmarkModLauncher.Posting.Mixed.postMixed avgt 5 6.687 ± 0.141 ns/op
BenchmarkModLauncher.Posting.Mixed.postMixedDozen avgt 5 81.762 ± 4.055 ns/op
BenchmarkModLauncher.Posting.Mixed.postMixedHundred avgt 5 940.126 ± 39.862 ns/op
BenchmarkModLauncher.Posting.Static.postStatic avgt 5 5.510 ± 0.030 ns/op
BenchmarkModLauncher.Posting.Static.postStaticDozen avgt 5 71.884 ± 1.965 ns/op
BenchmarkModLauncher.Posting.Static.postStaticHundred avgt 5 1072.279 ± 34.315 ns/op
After:
Benchmark Mode Cnt Score Error Units
BenchmarkModLauncher.Posting.Dynamic.postDynamic avgt 5 5.678 ± 0.035 ns/op
BenchmarkModLauncher.Posting.Dynamic.postDynamicDozen avgt 5 76.064 ± 0.588 ns/op
BenchmarkModLauncher.Posting.Dynamic.postDynamicHundred avgt 5 1104.990 ± 16.312 ns/op
BenchmarkModLauncher.Posting.Lambda.postLambda avgt 5 8.355 ± 0.085 ns/op
BenchmarkModLauncher.Posting.Lambda.postLambdaDozen avgt 5 86.770 ± 5.007 ns/op
BenchmarkModLauncher.Posting.Lambda.postLambdaHundred avgt 5 2231.929 ± 82.899 ns/op
BenchmarkModLauncher.Posting.Mixed.postMixed avgt 5 5.650 ± 0.012 ns/op
BenchmarkModLauncher.Posting.Mixed.postMixedDozen avgt 5 77.397 ± 2.974 ns/op
BenchmarkModLauncher.Posting.Mixed.postMixedHundred avgt 5 918.289 ± 16.768 ns/op
BenchmarkModLauncher.Posting.Static.postStatic avgt 5 5.588 ± 1.002 ns/op
BenchmarkModLauncher.Posting.Static.postStaticDozen avgt 5 74.352 ± 2.202 ns/op
BenchmarkModLauncher.Posting.Static.postStaticHundred avgt 5 1082.303 ± 53.286 ns/op
|
|
||
| import net.minecraftforge.eventbus.api.IEventListener; | ||
|
|
||
| interface IReactiveEventListener extends IEventListener { |
There was a problem hiding this comment.
This causes reactive event listener implementations to have two abstract methods, which makes it harder for the JVM to perform some lambda-specific optimisations that expect a single abstract method implemented (exceptions are made for overrides of Object and sometimes default methods too).
As this is package-private, you can replace it with a static helper method in ReactiveEventListener that does an instanceof switch instead.
There was a problem hiding this comment.
EventListeneres have two methods in the root. Is it just an issue that it's not defaulted?
Also, no cases use lambdas for implementation?
I could defauly this method to just return this; if its actually a performance issue.
| ListenerList listenerList = getListenerList(eventClass); | ||
| var cancelable = listenerList.isCancelable() || EventListenerHelper.isCancelable(eventClass); | ||
|
|
||
| IEventListener finalListener = ReactiveEventListener.of(listener, listener.toString(), genericFilter, receiveCancelled, cancelable); |
There was a problem hiding this comment.
This causes wrapping still as the IEventListener is first created as an anon class on line 241 and then wrapped in ReactiveEventListener.Unchecked.
Been a while since I last checked, but I think anon classes that capture a local var like this have their backing field trusted by MS OpenJDK, as it's smart enough to figure out that you can't reflect on the field easily.
There was a problem hiding this comment.
Ya, lambdas can be implemented in multiple ways by the compiler, These anon classes being one of them.
The reason i wanted to have this as an anon class is because the toString method in current implementation is completely useless. So when we have errors in events, it literally just shows that its all EventBus lambda listeners. This way it points to the lambda passed in by the modder which should have their class name in it and thus we have a far easier time debugging it.
Even if it is slightly slower, which I doubt, its worth it. Unless its like a 50x difference.
| private static abstract class Base implements IEventListener { | ||
| protected final IEventListener listener; | ||
| protected final String readable; |
There was a problem hiding this comment.
The use of instance finals here doesn't hurt compared to the existing ASMEventHandler which did the same, but does hurt the lambda posting path as they're no longer trusted (previously it was capturing lambdas in the EventBus class, which were trusted finals due to being in hidden classes coming from LambdaMetaFactory).
ASMEventHandler#of's existing method now wrapping this is a shame, but on the bright side ReactiveEventListener.Base can become an interface and its implementations records, but then you run into having a second non-abstract public method in the record for the toString() to use which causes the same issue as https://github.com/MinecraftForge/EventBus/pull/99/files#r2432897093...
With IReactiveEventListener being replaced with an instanceof helper method it could still be a net benefit, as it'll be the same as how this PR currently is but with trusted finals and an improvement over what's in the 6.2.x branch atm as the bulk registered listeners would get trusted too.
There was a problem hiding this comment.
Not exactly sure what you mean, but part of my answer is the same as why I use that anon class. I want to have a good error message when things break.
I did have a copy of all these classes in ASMEventHandler in the first pass, because I didn't notice lambdas.
I deleted them and pushed them through this to prevent the duplicate classes. With just the super type changed from Object to ASMEventHandler.
What type of instanceof helper are you expecting if there is no central interface because it introduces another abstract method? We'd have to add an instanceof check for every internal subclass that needs conversion. In the current case its only 2 Unchecked and GenericReactive.
| if (EventListenerHelper.isCancelable(eventType)) | ||
| listenerList.setCancelable(); |
There was a problem hiding this comment.
I think moving the EventListenerHelper#isCancelable check inside the ListenerList#setCancelable method (and renaming it appropriately, i.e. setCancelableIfNeeded) would avoid redundant lookups when already set as canceled.
There was a problem hiding this comment.
The lookups already happen in the ASMEventHandler builder, but sure, I could move that.
There was a problem hiding this comment.
Actually on second thought I'll change the if to if (!listenereList.isCancelable() && EventListenerHelper.isCancelable(eventType) which will short circuit the lookup if its already set. This prevents the need to have a second setCancelable that does the check vs the one that forces it on the parent.
What would be good, but not sure how to do it performativly. Is having a ternary:
unchecked - We don't know what type we are and thus need to check the class
true - we know our type and it was cancelable
false - we know our type it was not-cancelable and we havent seen a child that is.
|
Officially handing this off to Paint as the main point of this was to illustrate an idea on how to achieve the same optimizations without having to rely on making the Event sub-classes final/sealed. Which is a breaking change for older versions. For documentation sake, the point I am trying to make with the lambda names is this: Note how the ASM handlers tell you which mod the handler is in. Where as the lambda one always points to For future note, the eventual goal is to downgrade the 6.2 branch back to Java 8 compatibility. So that when the time comes we can back port these changes to 1.16.5 and below. The best way to achieve that whilst also getting the benefits of modern Java is to turn this into a multi-release jar. Which is a bigger task then i feel like doing today, and is not needed until we restore Java 8 compatibility. Which was dropped in 5.0 because of hard dependency on modlauncher. It is important to note tho, that 6.x in theory is purely additive API wise to previous versions all the way to 1.0. Which is why I would prefer to have a single v6 legacy branch and the modern v7 branch. Opposed to a v6, v5, v4, etc... |
|
If that's the case, we should still use some tool that dumps the public API of this library for each major version. The only problem I see with updating this on even older versions is that Minecraft projects are no stranger to shitty reflection practices. That's a bridge we can cross when we get there, though. And to anyone else reading this: again, the last thing we want to do is break compatibility. Everything we aim to do with older versions will be additive. |
3 participants
As discussed on discord, there are some issues with the current approach when it comes to making events final/sealed in older version: MinecraftForge/MinecraftForge#10685
This approach takes a more lazy approach by making the ASMEventHandler as naive as possible based strictly on the event type itself.
And then we sub-events are registered it will transform the handler to have the checks that are needed.
This should also does not require any information about child events during registration such as the class being final/sealed. So it should be compatible with all old versions.