Adds IEvent interface for Record event support #56
Conversation
PaintNinja
left a comment
PaintNinja
left a comment
There was a problem hiding this comment.
Thanks for your PR. Did a quick code review on my phone, looks good. Assigning to Lex for further testing.
In terms of performance, I expect record component getters to be faster than normal class getter methods, but event posting and subscribing to be around the same as normal classes. Even if performance ends up being the same, having the record syntax for some events would be nice.
|
|
||
| static ListenerList getListenerListInternal(Class<?> eventClass, boolean fromInstanceCall) { | ||
| if (eventClass == Event.class) return EVENTS_LIST; // Small optimization, bypasses all the locks/maps. | ||
| if (eventClass == Event.class || eventClass == IEvent.class || eventClass == Object.class) return EVENTS_LIST; // Small optimization, bypasses all the locks/maps. |
There was a problem hiding this comment.
Why is Object.class added?
There was a problem hiding this comment.
When I was running the tests I kept getting a weird error where it was trying to cast Object to IEvent when getting the Listener list. It's because the get parent call returns the class it extends, not the interface it implements so the record test class' parent was Object
There was a problem hiding this comment.
Odd... The record's parent class should be java.lang.Record as that's what all record classes extend.
There was a problem hiding this comment.
Yes, this is what I thought at the time especially because when looking at it in ClassNode that's what it says. However, the Class<?> superclass = eventClass.getSuperclass(); call was giving Object (otherwise I assume I would've seen the same casting exception but for Record)
|
Benchmarks take a long time because it tests on a huge amount of JVMs by default iirc. You can speed this up by changing the |
My assumption was that the record events might gain some performance from the default implementation on methods such as Either way, I'll re-run them on a smaller sample size as you suggested and get back with the results. |
|
Superseded by #69, which completely redesigns EventBus. |
3 participants
Adds, and migrates all references, to the IEvent interface to allow creating record based events in the future.
I'm not sure if this was the correct way to do this, but I ran the tests against everything I changed, and added one for the Record events because I wasn't sure it'd work with phases, and they passed. I was going to run the performance tests, and see if there was any notable difference for records, but it was giving a 12 hour ETA so I decided against doing that.