Reimplement Events using the Observable pattern
yyny opened this issue ยท 9 comments
Events are useful for interacting with Minecraft in a portable way. Unfortunately, events in their current form are unsuitable for the Observer pattern, since events can consist of multiple arguments and don't provide a direct way to modify the event as it is being propagated to listeners. I suggest reimplementing all Events to conform to the Observer pattern, by placing the input arguments into a record-like class. Note that this suggestion applies to the internal implementation only, the external API can remain the same (Although obviously, we should allow mods direct access to the record-like if they so desire, and perhaps the multi-argument lambda functions should eventually be removed by a deprecation cycle, although that is a separate issue).
A couple of advantages for Observables over Events:
- Additional methods can be added for convenience or additional functionality (a
Event#cancel();
or e.g.ChatMessageEvent#changeMessage(message -> toUpperCase(message));
) - Adding additional inputs or methods later on will not break any existing code.
- Generating events or event listeners is much simpler, and doesn't require Reflection or ASM,
@Annotations
would suffice. - All Observable operators can be applied as usual, e.g.
WorldTickEvent.filter(event -> isOverworld(event.getWorld()))
. In the current system, operators likemap
cannot be implemented.
In general, Observables are much nicer to deal with because of this. - We can provide native ("hardware-accelerated") support for common operations, e.g. we could implement the above filter as
WorldTickEvent.filter(OVERWORLD_FILTER)
, such that if 20 mods apply this filter, the code for it will only have to be invoked once per event, instead of 20 times using 20 differentif
statements. - Different parts of a listener can be executed asynchronously on different threads if desired, much like
.stream()
in Java.
A couple of concerns debunked:
Q: Doesn't wrapping inputs in record-likes create unnecessary allocations?
A: No. The record-likes can be cached between listeners, and between events for non-recursive events (Which is most events). This is also why it is important for Fabric to provide this functionally itself, because other mods can't reliably cache events themselves (without mixing into Fabric, of course), meaning that even if an external mod were to implement Observables for themselves by manually wrapping the inputs into a record-like, the implementation would still be much less efficient.
Q: Can't listeners unexpectedly mutate the events they are passed?
A: No. record-like members can be made final, or record-likes can instead be implemented as interfaces with getter methods.
Q: Aren't Observables much slower than the current system?
A: No. Internally, Observables are implemented the same way as the current system, with the only difference being that only listeners that implement Consumer<T>
are allowed.
Listeners can cache the inputs into their own local variables, meaning the overhead per event only has to be a single GETFIELD
copy per listener, and you have to remember that this copy overhead already exists in the current system because the arguments have to be copied to the stack for every method invocation. Observer listeners only have to copy the fields that they actually use, whereas the current system always needs to copy all inputs for every listener invocation.
Q: How can we cache record-likes if their contents are final?
A: We can use ASM or Reflection to change the contents once per event.
Q: Registering a lambda with multiple arguments is much more convenient!
A: We can still keep supporting the old system with minimal overhead.
Q: Wouldn't implementing an entire Reactive Library for fabric require lots of maintenance?
A: Although it would be nice for performance tuning, Fabric doesn't strictly have to provide any Observable methods itself, libraries like RxJava provide adapter methods to turn Events into propper Observables. The only change that is required to come from fabric is to limit listener inputs to one object.
Generating events or event listeners is much simpler, and doesn't require Reflection or ASM,
@Annotations
would suffice.
I bet you haven't read forge event bus. Annotation detections and method calls through reflection are unintuitive and slow. Forge uses asm to generate a class dedicated to call events to avoid reflection.
Also your "cache" of recorders is even more dumb. In modern java, final fields in pojos enjoy from jvm optimizations and additionally, modern gc is smart.
In the current system, operators like
map
cannot be implemented.
Example please. Don't understand what you are saying here.
After all, I don't think observer is single pojo that contains all event data, like forge events. I recall observer is more like fluent iterable or stream or completable future. it doesn't have anything to do with fabric events.
Also for your hardware acceleration, we already are doing different event instances for different sites. Those imo are significantly clearer and better than your "native optimizations".
In your observable/stream argument, you say that stream operations are good. No, they aren't; they are costly.
For instance, java 8's stream is only good for these circumstances:
- early quit, where you call like
anyMatch
(early quit on true)allMatch
(early quit on false)findAny
findFirst
- parallel stream, where you iterate stuff efficiently
Another probability is you want to get rid of intermediate collections, which may be a useful point as well. But beyond those, the streams, along with the observable pattern, has zero advantage.
modern GC is smart
[...]
Also for your hardware acceleration, we already are doing different event instances for different sites. Those imo are significantly clearer and better than your "native optimizations".
I agree, but I've seen memory allocation and performance concerns several times, so I wanted to address them. I personally think it would be better Java style to allocate one event object per event and let Java GC deal with the allocation overhead.
Annotation detections and method calls through reflection are unintuitive and slow.
Annotations can be checked at class-load time, no? Mixins use annotations in the same way, worst case we add a new "events" entrypoint for initializing events and prohibit annotations outside of those entrypoints, just like with Mixins (I actually think that would be a pretty clean solution, it would also group up all event initialization code into a single class/classes). Anyway, being able to use annotation to register listeners isn't really part of this proposal and just a nice side effect of single-argument listeners and being able to add/remove/transform event listeners at will.
Forge uses asm to generate a class dedicated to call events to avoid reflection.
Seems like a bad way to go about things yes, but why would we have to do the same?
I recall observer is more like fluent iterable or stream or completable future. it doesn't have anything to do with fabric events.
An Observable is a "stream of events", an "Observer" is the lambda/interface/object listening for those events. I would say that makes it quite relevant for implementing an event system in fabric. Observers are registered to Observables and get notified with the new state of the Observable whenever it changes. The difference between "event listeners" and "observables" is that observables compose. In other words, we can efficiently combine Observables, perform operations on observables, etc without having to define new Event<T>
s every time. Another difference is that Observables can be (implicitly) canceled, i.e. when an event is no longer interesting or required, it can be optimized away. Try to write a function to registers an event listener that gets invoked only once using the current event system in fabric.
Observable<T>
is to CompletableFuture<T>
what Iterator<T>
is to Supplier<T>
, it's just a way to asynchronously return multiple values, the asynchronous version of Iterator
. You could replace Observable
s with Events in the same way that you can replace Iterator
s with Arrays, the code is a little simpler like this for trivial use cases, but there is also much less room for optimizations and more complicated use cases become hard to implement (efficiently, if at all). You can combine multiple Iterator<T>
s into a new Iterator<T>
and still have the same behaviour as invoking the iterators separately one after another, but you can't combine Supplier<T[]>
s like that, you would have to wait for all the arrays to have been fully created or allocate a proxy class that keeps track of which arrays have been created and which have yet to be, at which point you wouldn't have a Supplier<T[]>
anymore (i.e. the operation is non-composable). The same kind of logic applies to Observables vs event listeners.
Example please. Don't understand what you are saying here.
We can't implement a function that maps an event of one type to an event of another type (static Event<U> convert(Event<T>, Function<T, U> converter);
), because Java functions cannot return multiple values (Which is needed because in the current implementation in fabric, the state of certain events is represented using multiple values).
In your observable/stream argument, you say that stream operations are good. No, they aren't; they are costly.
I don't know where you get this from. Perhaps this is true for Java .stream()
s, but there exist Observable libraries that are specifically optimized to make these operations as fast as possible. Also, I'm not arguing fabric-api users should be using observable operators but that they should be able to.
But beyond those, the streams, along with the observable pattern, has zero advantage.
This is objectively false, it is impossible to predict the value that the observer pattern might bring to projects using fabric-api. It could be argued that the disadvantages outweigh the advantages, but I have already outlined the few disadvantages there are, meanwhile the advantages grow exponentially with every project interested in an implementation of observable events. Remember that without an unifying observable interface, each of these potential projects would have to implement an observable implementation separately. Even assuming this can and will be done efficiently, there would still be many missed inter-project optimizations.
Because there seems to be some confusion regarding what the proposal here actually is, my proposal is to implement a minimal implementation for the observable pattern, enough to efficiently start using the observable pattern in project code or use an observable library. From what I have gathered so far, the features missing are:
- The state of every event must be representable as a single object or interface.
- Event subscriptions must be cancellable.
- Events must have a well defined failure and finish condition (Probably a NOOP since from what I can tell fabric events can never fail or finish).
I am having a hard time understanding what you are suggesting... Would you mind providing a substantial example to highlight the advantages this observer approach would have, i.e. a > 10 LOC fragment detailing how your suggested new API would be used, and then you can explain how it's better? Ideally using an existing widely used fabric event (e.g. a lifecycle event), and comparing with the existing event system.
The record-likes can be cached between listeners, and between events for non-recursive events (Which is most events).
No they can't, if they are, then storing event instances will cause issues, for example if a mod stores the event object in a static field or list, it will appear to mutate even if the event is not touched, this isn't intuitive at all
Adding additional inputs or methods later on will not break any existing code.
we already have a mechanism for this, you could easily make a new interface with the new parameters, and make the old interface implement the new one, delegating the method
Additional methods can be added for convenience or additional functionality (a Event#cancel(); or e.g. ChatMessageEvent#changeMessage(message -> toUpperCase(message));)
this is not more convenient, it means this has to be special cased, this is already possible in the current API because the current API allows for flexible return type handling, something an object based system does not
Aren't Observables much slower than the current system?
Yes, they are, and I've benchmarked it
https://github.com/Devan-Kerman/NanoEvents#event-objects
We can use ASM or Reflection to change the contents once per event.
Modifying final fields at runtime with reflection is undefined behavior iirc and it's not something we should rely on
Generating events or event listeners is much simpler, and doesn't require Reflection or ASM, @annotations would suffice.
the current system is simple enough, writing a for loop is not that hard, and there is plans to automate the process already. And magic annotations are most definitely not the solution. Mixin gets a pass cus it's basically it's own language that has to work within java.
We can provide native ("hardware-accelerated") support for common operations, e.g. we could implement the above filter as WorldTickEvent.filter(OVERWORLD_FILTER), such that if 20 mods apply this filter, the code for it will only have to be invoked once per event, instead of 20 times using 20 different if statements.
this is already possible, just create a new event handler that registers itself as a listener in the old listener and filters it
Different parts of a listener can be executed asynchronously on different threads if desired, much like .stream() in Java.
this is also already possible, the invocation is handled by the user, they can invoke however they like
We can't implement a function that maps an event of one type to an event of another type (static Event convert(Event, Function<T, U> converter);), because Java functions cannot return multiple values (Which is needed because in the current implementation in fabric, the state of certain events is represented using multiple values).
dunno what u mean by that, but in the case of multiple return types we can have return objects or event objects, in all other cases, which is most of them, it's unessesary
Java functions cannot return multiple values
int[]
return multiple integers noises go brrr
Please, I want to see actual code, I don't think throwing around concepts and arguments like this is very useful. ;-)
You're currently proposing a change that would fundamentally break the entire event architecture of Fabric. While I'm not fundamentally opposed to fundamental reworks, you have yet to provide any example code of how this would work, much less an analysis of its performance impact. This is a forge-y way of doing events that is incompatible with the Fabric ethos. If you really want this, bring it back as a PR so we can audit and test it. I'm using my authority as a member of the triage team to close this, as further bickering won't do anything unless we have real code to inspect.
Yeah, you can open a draft pr like you did for FabricMC/yarn#1940