Add Event#hasListener, remove unused junk

[apple502j]

Should be self-descriptive.

[modmuss50]

Please add some tests.

[Technici4n]

IMO this is a bad pattern. If you are concerned about allocation, reuse the context object. In typical modpacks, most events will be used and these checks will be pointless anyway.

[modmuss50]

Thats a fair point, @apple502j did you have a specific use case in mind?

[BasiqueEvangelist]

A usecase I had for this was for a deprecated event, where actually properly servicing it required setting up some expensive stuff

[apple502j]

Also, not all context objects can be reused.

[Technici4n]

Can you link some specific examples?

[BasiqueEvangelist]

https://github.com/BasiqueEvangelist/ne-v-seti/blob/sanity-1.17/src/main/java/me/basiqueevangelist/nevseti/OfflineDataCache.java#L30
I guess I misremembered specifically how much it would take to service this event, but I think it will still be useful for events that are deprecated and require running extra logic to run their invokers

[Technici4n]

This is not resolved, please don't mark it as resolved.

[modmuss50]

Still wating on a good usecase for this, in places where it matter we already re-use a context object, and firing an event with no listeners should have little overhead.

[apple502j]

I think I had some events in my mind when I filed this PR, I need to figure that out again.

[apple502j]

Events with non-zero invocation cost

• RegSync: Remap event constructs RemapStateImpl. Very negligible as this is called only once per connection.
• ItemGroupEvents: Involves creation of several objects. Once per startup, so doesn't really matter, unless Mojang decuples the number of items or something.
• LootTableEvents: This might benefit from a check. An event (invoked per loot table) requires LootTable to be converted back to builder.

Will do a more thorough research later.

[melontini]

LootTableEvents: This might benefit from a check. An event (invoked per loot table) requires LootTable to be converted back to builder.

IMO this would only benefit if there was a separation between singular and wildcard events (like MODIFY_ALL, modify(RegistryKey) or modify(Predicate<LootTable>)).

With the current implementation, if a single mod wants to modify a single loot table, all the benefits of such a check disappear.

[Juuxel]

modify(Identifier) or modify(RegistryKey<LootTable>) would actually make sense for the API since most modifications are probably targeted to just a single loot table.

[ishland]

When a synchronous event that's only meant to be invoked from the server thread needs to be invoked from a worker thread, it needs to make a round-trip to the server thread. Such round-trip can take 50ms or more depending on the MSPT of the server.
Wasting that amount of time invoking an event that nobody listens to is just a waste of time. Having such API can avoid doing such costly round-trips.

I encountered this when implementing #4541 where I have to make round-trips to the server thread to invoke the event:
RelativityMC/C2ME-fabric@0a04fdc
There's one such case in the ServerBlockTicking class and another two in ServerEntityTicking.

[Technici4n]

Wouldn't this indicate a flaw with the event design? The moment any mod registers a listener, all the benefits of C2ME will be gone, no? @ishland

[ishland]

Wouldn't this indicate a flaw with the event design?

I can't think of any better designs other than splitting that event into 6 events. Relaxing where the event can be invoked just adds more trouble for modders that have no idea about threading.

The moment any mod registers a listener, all the benefits of C2ME will be gone, no?

No. Multiple pending synchronizations with the server thread can be in flight at the same time. But adding extra server thread synchronization points (for just invoking the event) adds more server thread overhead and increases latency.

[modmuss50]

I dont have any major concers with this, the use case seems good enough to me. The PR will need updating before moving forward.