Full rewrite and redesign

[PaintNinja]

This PR is a full rewrite of Forge's EventBus, a conclusion to the NovaBus experiments worked on-and-off for the past
year (first started in late February 2024), incorporating some of the requested API designs discussed on the Discord.

Table of contents

• What's NovaBus?
• Why a full rewrite?
• List of changes
• Removed features
• WIP/Todo
• Compatibility
• Performance techniques
  • Posting
  • Registration
• Benchmarks
  • Posting
    • Dozen
    • Hundred
    • Single
    • Scaling
    • Posting data
  • Registration
    • Registration data
• Future work
• Looking far ahead

What's NovaBus?

The codename for my experiments with EventBus. Over the past year, I've been playing around with many different ideas
and designs with others, learning a lot about the JVM and Java along the way. A small subset have already been
backported into Forge's EventBus, such as #58, #63 and #65. But there's a lot that can't be done without breaking
changes, which is what this PR is for.

To be clear, the codename is mainly to avoid confusion and verbosity when comparing v6 and v7. When I say NovaBus here,
I mean the new EventBus v7 which this PR represents. The name of the EventBus project remains unchanged.

Why a full rewrite?

A good portion of the experiments were focused on maintaining partial backwards-compatibility with the current EventBus
and not making too many breaking changes. By the time we were finished, there was huge implementation complexity and
only a handful of desired features and minor performance improvements to show for it...

It became apparent that more meaningful improvements (in every metric - performance, API, impl...) would only be
possible when freeing ourselves from the constraints of the current design. After further discussion on Discord, the
decision was made to drop various unwanted features and to go ahead with a full rewrite.

List of changes

• Cancellation and monitoring are decoupled from the event instance
• Listeners can be Consumer, Predicate, or ObjBooleanBiConsumer, the latter two for cancellable events
  • For cancellable events, listeners can declare they always or never cancel the event, allowing for additional
    optimisations
• CancellableEventBus extends EventBus to avoid breaking changes for non-cancellable listeners relying on events that
  change from cancellable to non-cancellable
• ListenerList and EventBus have been merged. Each Event has its own EventBus. An Event can have multiple
  EventBus instances, but each EventBus can only have one Event. A group of EventBuses is called a BusGroup.
  Here's a rough mapping from old to new:
  • ListenerList + EventBus -> EventBus
  • EventBus -> BusGroup
  • BusBuilder -> EventCharacteristic interfaces
• Strong encapsulation, taking full advantage of the Java module system and sealed types to better organise the system
  and allow flexibility for future internal changes without the risk of breaking the API. Strong separation of API spec
  and implementation details.
  • Only the api packages are exposed, the rest is internal
  • API is split into three packages based on purpose:
    • event for creating events
    • listener for creating listeners
    • bus for registering listeners, posting events and managing buses
• Forward-thinking design which pre-emptively accounts for Valhalla value classes, null-restricted types, etc...
  • Allows for further performance improvements in the future as the JVM evolves, without breaking changes
  • jSpecify is used for enforcing null-restricted types today
• Event is now a sealed interface to support records and multiple inheritance
• Event characteristic annotations have been replaced with interfaces to allow for more granular control (no
  annotation restrictions/limitations)
  • Inheritance support is both opt-in and opt-out
    • Opt-in is done by implementing the InheritableEvent interface
    • Opt-out is done by annotating an event that extends InheritableEvent with @MarkerEvent
  • Phase tracking is now only available for the Monitor phase and is opted in with the MonitorAware interface
  • Events can declare they are only fired once (EventCharacteristic.SelfDestructing) to automatically dispose of
    themselves after being posted to save memory
• Each EventBus determines its functionality based on the Event it's associated with and its registered listeners
  • The event isn't cancellable? Skip checking if the event was cancelled
  • The event is cancellable, but all listeners are Consumers? Can also skip checking if the event was cancelled
  • Whole specialised invokers are generated, rather than only the array of listeners
• Priority is now a byte instead of an enum, larger numbers run first, with 0 being the default. Decided on a byte
  instead of int because:
  • Needing more than a couple of hundred priorities is likely a sign of bad code (this is the main reason)
    • CSS z-index: 999999 moment
  • Priority comparisons are simpler and faster, especially considering any boxing is more likely to be covered by the
    JVM's preloaded autoboxing cache than an Integer
  • It's more friendly to Valhalla value classes and memory usage
• receiveCanceled has been removed for normal listeners, monitoring listeners have it implicitly
• Bulk registration
  • Bulk registration of listeners uses LambdaMetaFactory instead of manual class generation and transforms
  • Attempting bulk registration on a class containing no listeners will throw an exception to prevent accidental
    registration of the wrong class
  • Supplying an instance of a class will now also register static methods. No more artificial restrictions when
    registering instance vs static methods
  • Much more comprehensive validation to avoid silent failures when library users make mistakes
• Inheritance support is aware of sealed hierarchies to reduce memory usage
  • If the event is final (such as for implementations of RecordEvent), the child list will be a shared empty list
  • If the event is sealed, the child list will be exactly sized based on the number of permitted subclasses
• Events can be self-posting, allowing for more concise code in some cases (new MyEvent().post())
• BusGroup supports trimming the backing lists of all loaded EventBuses in the group to save memory

Removed features

• receiveCanceled for normal listeners (use monitoring listeners instead)
• Generic events (add a Class<?> field/record component to your event or use event inheritance)
• Per-post listener filtering
• HasResult (use a nullable Boolean result field/record component instead, or make your own enum type)
• Phase tracking for anything other than the Monitor phase
• Inherited listeners in bulk registration

WIP/Todo

This is stuff I'd like some help with and/or plan to do in the near future:

• Nicer errors when doing bulk registration
• Test for possible concurrency issues in BusGroup
• Integrate into Forge (see MinecraftForge#10495 for a PoC)
• Better documentation

Compatibility

This is a full rewrite that deserves a major version bump. The underlying design is different enough that most existing
code wouldn't work without changes even if the packages and classes had the same names. I decided that it's better to
do one big breaking change now than to annoy developers with a lot of spread-out, smaller breaking changes.

Due to the scale of change, it might be good to merge this as EventBus 7.0.0-beta.1 or similar, to allow a short
breaking change window just in case we find anything important that needs fixing while integrating into Forge.

We should make a separate branch for EventBus 6.x so that we can continue maintaining it.

Performance techniques

Event dispatch is specialised based on each event's capabilities, its listeners' capabilities, the number of listeners
and available JVM features. Invokers are generated lazily and cached, with careful consideration for JVM performance
pitfalls. The code for posting hotpaths has been iterated on a lot overtime, driven by benchmark results.

Posting

In the case of zero listeners, EventBus v7 is able to dynamically instruct the JVM that the event dispatch is no-op and
is optimised away entirely by the C2 JIT. The performance is the same as if the event was never instantiated or posted,
effectively resulting in zero overhead for unused events without requiring special handling from library users.

Events with a single listener are resolved to a direct method call, skipping any redundant cancellation checks and loops.

Depending on the size of the compiled method bytecode for lambdas, a small number of listeners can be preloaded into
trusted finals with a manually unrolled loop. This allows for additional optimisations by the JVM that typically
wouldn't be possible with a normal array of listeners, without eating up too much of the inlining budget.

Cancellation checks are only performed if the event is cancellable and at least one listener possibly cancels the event.

If all listeners are known to never cancel the event, we can treat it as if the event isn't cancellable until a
possibly cancelling listener is registered later.

If a listener always cancels, we skip remaining listeners at invoker build time, reducing memory accesses and allowing
for some of the size-specific optimisations mentioned above to still apply even when the number of listeners would
otherwise exceed it.

There are additional tricks involved, but I think you get the idea... ;)

Registration

A lot less indirection is involved in registration now and the new design of registering directly to events removes the
need for TypeTools. Adding individual listeners is basically wrapping the lambda in an EventListener instance and
adding it to a synchronized collection... that's pretty much it.

Bulk registration is no longer an entirely separate system - it's now a frontend for the same system used for individual
registration. It takes a class, does some reflection to list its methods, performs some sanity checks, converts them to
direct lambdas with LambdaMetaFactory and then registers those as if they were individual listeners. The class
generation is all done for us by the JDK and is much more efficient. Still slower than individual registration because
it shifts a bunch of work that would otherwise be done at compile-time to runtime, but that might be something we can
improve with compile-time transformers in the future.

Inheritance support is handled differently to significantly reduce the amount of work involved in building the sorted
list for the invoker. The child's backing list is preloaded with the parent's listeners if the event is inheritable.
After the EventBus is built, the parent EventBus is notified of the new child and future listeners added to/removed from
parents propagate to their children. Sorting is done directly on the backing list, so resorting is relatively cheap.

Benchmarks

EventBus v7 offers significant performance improvements across the board in like-for-like benchmarks, no transforms
needed. I encourage reviewers to also run the benchmarks themselves to provide additional data points.

Posting

Dozen

Benchmark	EventBus v6 NoLoader	EventBus v6 ModLauncher	EventBus v7 (NovaBus)
Dynamic dozen	158.569 ns/op ± 2.755	156.574 ns/op ± 11.053	103.117 ns/op ± 1.346
Lambda dozen	140.146 ns/op ± 7.550	119.641 ns/op ± 0.839	95.778 ns/op ± 2.403
Static dozen	137.157 ns/op ± 18.634	125.666 ns/op ± 22.419	95.705 ns/op ± 0.854

Compared to EventBus v6 in ModLauncher mode, NovaBus is about 35% faster for dynamic listeners, 20% faster for lambda and
25% faster for static listeners. The improvements are of course even larger when comparing both in NoLoader mode.

Hundred

The numbers are less consistent when there's so many listeners, but NovaBus still manages to be within the same ballpark
of improvements as with a dozen listeners:

Benchmark	EventBus v6 NoLoader	EventBus v6 ModLauncher	EventBus v7 (NovaBus)
Dynamic hundred	4710.419 ns/op ± 221.955	4281.470 ns/op ± 522.737	3420.696 ns/op ± 237.921
Lambda hundred	4277.721 ns/op ± 71.710	3891.848 ns/op ± 772.864	3464.047 ns/op ± 195.707
Static hundred	4344.508 ns/op ± 122.697	4036.750 ns/op ± 222.120	3393.881 ns/op ± 198.007

The relative performance difference between dynamic and static listeners has also reduced enough that I no longer
consider it worth thinking about from a performance perspective, especially combined with the gains over the previous
design.

Single

For single listeners, we can see NovaBus has allowed the JVM to optimise away the event dispatch entirely:

Benchmark	EventBus v6 NoLoader	EventBus v6 ModLauncher	EventBus v7 (NovaBus)
Dynamic single	28.671 ns/op ± 0.239	17.236 ns/op ± 0.111	0.251 ns/op ± 0.026
Lambda single	26.076 ns/op ± 0.313	17.118 ns/op ± 0.237	0.244 ns/op ± 0.014
Static single	25.670 ns/op ± 0.306	16.472 ns/op ± 0.222	0.243 ns/op ± 0.011

This is intentional. The new design ensures a fully inlinable execution chain for upto a handful of listeners, which
allows the JVM to see that although a listener is registered, the listener method is no-op. The performance is the
same as an empty JMH benchmark method - it's as if the event was never instantiated and posted.

To ensure a like-for-like comparison, I did not change the existing benchmarks. There are real-world cases where people
accidentally register empty event listeners, so it's good to show that the new architecture can accommodate this.

I did look into solutions which avoid inlining, such as using JMH blackholes in all the events and consuming the event
instance from each listener, however this isn't straightforward to do in a way that reflects real-world usage with all
listeners not being no-ops. For instance, the blackhole approach prevented inlining of event instantiation for events
with constant values. Another approach was to use a @CompilerControl annotation to prevent inlining, but this only
worked for the initial event instantiation and not for the listener method calls.

We can reconsider the benchmarks in a future PR if needed. For reference, I got around 5ns/op for the single dynamic
listener case with the Blackhole approach and around 1ns/op with the CompilerControl approach, but I'm not confident
in the accuracy of these inlining mitigation numbers at this time and didn't apply those approaches to the other
benchmarks (and thus are not directly comparable).

Scaling

The biggest wins come from when you have only a handful of listeners at most. The zero overhead for posting unused events
is especially nice.

Sidenote

Why are the posting benchmark numbers so different from the December preview shown on the Discord?

This is to do with the handling of duplicate listeners. In the December preview, listeners were not deduplicated by
NovaBus, and I confirmed the number of listeners was as expected, therefore, I didn't use the ClassFactory as I thought
it was unnecessary. This was further confirmed by the SubscriberLambda posting benchmarks performing similarly without
the ClassFactory for EventBus v6 and the remark/comment I remember seeing somewhere about how lambda listeners are not
deduplicated.

After further investigation, I found that while the number of listeners was indeed correct and not deduplicated, the
JVM was smart enough to tell that they all referred to the same static method and performed additional optimisations.
Note that these optimisations were not on NovaBus' end and explicitly deduplicating still provided much better results.

So technically, both the December preview and the current version have accurate benchmark numbers, but the December
preview was less representative of real-world performance. The December preview benchmarks acted as if each mod was
doing addListener(LibraryMod::onEvent) instead of actually unique methods specific to each mod
(like addListener(ModA::onEvent), addListener(ModB::onEvent), etc...). The current version uses unique methods for
each listener.

The numbers are fair in both versions because I did like-for-like comparisons with Forge EventBus without any explicit
deduplication. In other words, the December preview benchmarks also skipped the ClassFactory for Forge EventBus and
both used the SubscriberLambda suite, so the numbers were still comparable. I opted to use the ClassFactory again for
this PR's numbers for more realistic results.

Posting data

(Click to show/hide)

EventBus v6 benchmark                                    Mode  Cnt     Score     Error  Units
BenchmarkModLauncher.Posting.Dynamic.postDynamicSingle   avgt    5    17.236 ±   0.111  ns/op
BenchmarkModLauncher.Posting.Dynamic.postDynamicDozen    avgt    5   156.574 ±  11.053  ns/op
BenchmarkModLauncher.Posting.Dynamic.postDynamicHundred  avgt    5  4281.470 ± 522.737  ns/op

BenchmarkModLauncher.Posting.Lambda.postLambdaSingle     avgt    5    17.118 ±   0.237  ns/op
BenchmarkModLauncher.Posting.Lambda.postLambdaDozen      avgt    5   119.641 ±   0.839  ns/op
BenchmarkModLauncher.Posting.Lambda.postLambdaHundred    avgt    5  3891.848 ± 772.864  ns/op

BenchmarkModLauncher.Posting.Mixed.postMixedDozen        avgt    5   157.652 ±  15.551  ns/op
BenchmarkModLauncher.Posting.Mixed.postMixedHundred      avgt    5  4036.750 ± 222.120  ns/op

BenchmarkModLauncher.Posting.Static.postStaticSingle     avgt    5    16.472 ±   0.222  ns/op
BenchmarkModLauncher.Posting.Static.postStaticDozen      avgt    5   125.666 ±  22.419  ns/op
BenchmarkModLauncher.Posting.Static.postStaticHundred    avgt    5  4366.780 ± 477.861  ns/op

BenchmarkNoLoader.Posting.Dynamic.postDynamicSingle      avgt    5    28.671 ±   0.239  ns/op
BenchmarkNoLoader.Posting.Dynamic.postDynamicDozen       avgt    5   158.569 ±   2.755  ns/op
BenchmarkNoLoader.Posting.Dynamic.postDynamicHundred     avgt    5  4710.419 ± 221.955  ns/op

BenchmarkNoLoader.Posting.Lambda.postLambdaSingle        avgt    5    26.076 ±   0.313  ns/op
BenchmarkNoLoader.Posting.Lambda.postLambdaDozen         avgt    5   140.146 ±   7.550  ns/op
BenchmarkNoLoader.Posting.Lambda.postLambdaHundred       avgt    5  4277.721 ±  71.710  ns/op

BenchmarkNoLoader.Posting.Mixed.postMixedDozen           avgt    5   148.110 ±   1.983  ns/op
BenchmarkNoLoader.Posting.Mixed.postMixedHundred         avgt    5  3909.735 ± 151.824  ns/op

BenchmarkNoLoader.Posting.Static.postStaticSingle        avgt    5    25.670 ±   0.306  ns/op
BenchmarkNoLoader.Posting.Static.postStaticDozen         avgt    5   137.157 ±  18.634  ns/op
BenchmarkNoLoader.Posting.Static.postStaticHundred       avgt    5  4344.508 ± 122.697  ns/op

EventBus v7 (NovaBus) benchmark                (multiplier)  Mode  Cnt     Score     Error  Units
BenchmarkNoLoader.Posting.Dynamic.postDynamic             1  avgt    5     0.251 ±   0.026  ns/op
BenchmarkNoLoader.Posting.Dynamic.postDynamic            12  avgt    5   103.117 ±   1.346  ns/op
BenchmarkNoLoader.Posting.Dynamic.postDynamic           100  avgt    5  3420.696 ± 237.921  ns/op

BenchmarkNoLoader.Posting.Lambda.postLambda               1  avgt    5     0.244 ±   0.014  ns/op
BenchmarkNoLoader.Posting.Lambda.postLambda              12  avgt    5    95.778 ±   2.403  ns/op
BenchmarkNoLoader.Posting.Lambda.postLambda             100  avgt    5  3464.047 ± 195.707  ns/op

BenchmarkNoLoader.Posting.Mixed.postMixed                12  avgt    5   103.907 ±   1.803  ns/op
BenchmarkNoLoader.Posting.Mixed.postMixed               100  avgt    5  3450.128 ±  98.783  ns/op

BenchmarkNoLoader.Posting.Static.postStatic               1  avgt    5     0.243 ±   0.011  ns/op
BenchmarkNoLoader.Posting.Static.postStatic              12  avgt    5    95.705 ±   0.854  ns/op
BenchmarkNoLoader.Posting.Static.postStatic             100  avgt    5  3393.881 ± 198.007  ns/op

EventBus v6 scaling benchmark                     Mode  Cnt     Score    Error  Units
BenchmarkModLauncher.Posting.Static.postStatic0   avgt    5     2.602 ±  0.037  ns/op
BenchmarkModLauncher.Posting.Static.postStatic01  avgt    5    17.227 ±  0.213  ns/op
BenchmarkModLauncher.Posting.Static.postStatic02  avgt    5    28.435 ±  0.263  ns/op
BenchmarkModLauncher.Posting.Static.postStatic03  avgt    5    38.013 ±  0.286  ns/op
BenchmarkModLauncher.Posting.Static.postStatic04  avgt    5    46.752 ±  0.541  ns/op
BenchmarkModLauncher.Posting.Static.postStatic05  avgt    5    52.932 ±  0.206  ns/op
BenchmarkModLauncher.Posting.Static.postStatic10  avgt    5    99.195 ±  0.874  ns/op
BenchmarkModLauncher.Posting.Static.postStatic20  avgt    5   310.269 ± 16.706  ns/op
BenchmarkModLauncher.Posting.Static.postStatic30  avgt    5   688.963 ± 76.566  ns/op
BenchmarkModLauncher.Posting.Static.postStatic40  avgt    5  1139.787 ± 57.396  ns/op
BenchmarkModLauncher.Posting.Static.postStatic50  avgt    5  1565.009 ± 86.910  ns/op

BenchmarkNoLoader.Posting.Static.postStatic0      avgt    5     9.656 ±  0.028  ns/op
BenchmarkNoLoader.Posting.Static.postStatic01     avgt    5    28.350 ±  0.266  ns/op
BenchmarkNoLoader.Posting.Static.postStatic02     avgt    5    41.285 ±  0.626  ns/op
BenchmarkNoLoader.Posting.Static.postStatic03     avgt    5    51.022 ±  0.791  ns/op
BenchmarkNoLoader.Posting.Static.postStatic04     avgt    5    60.878 ±  0.863  ns/op
BenchmarkNoLoader.Posting.Static.postStatic05     avgt    5    72.092 ±  0.770  ns/op
BenchmarkNoLoader.Posting.Static.postStatic10     avgt    5   113.223 ±  0.576  ns/op
BenchmarkNoLoader.Posting.Static.postStatic20     avgt    5   296.146 ± 26.799  ns/op
BenchmarkNoLoader.Posting.Static.postStatic30     avgt    5   750.416 ± 52.972  ns/op
BenchmarkNoLoader.Posting.Static.postStatic40     avgt    5  1227.717 ± 78.786  ns/op
BenchmarkNoLoader.Posting.Static.postStatic50     avgt    5  1627.291 ± 92.041  ns/op

EventBus v7 (NovaBus) scaling benchmark      (multiplier)  Mode  Cnt     Score     Error  Units
BenchmarkNoLoader.Posting.Static.postStatic             0  avgt    5     0.252 ±   0.011  ns/op
BenchmarkNoLoader.Posting.Static.postStatic             1  avgt    5     0.253 ±   0.022  ns/op
BenchmarkNoLoader.Posting.Static.postStatic             2  avgt    5     0.249 ±   0.014  ns/op
BenchmarkNoLoader.Posting.Static.postStatic             3  avgt    5     0.248 ±   0.001  ns/op
BenchmarkNoLoader.Posting.Static.postStatic             4  avgt    5     0.250 ±   0.017  ns/op
BenchmarkNoLoader.Posting.Static.postStatic             5  avgt    5    40.256 ±   0.393  ns/op
BenchmarkNoLoader.Posting.Static.postStatic            10  avgt    5    80.401 ±   1.164  ns/op
BenchmarkNoLoader.Posting.Static.postStatic            20  avgt    5   170.603 ±  14.318  ns/op
BenchmarkNoLoader.Posting.Static.postStatic            30  avgt    5   404.331 ±  26.928  ns/op
BenchmarkNoLoader.Posting.Static.postStatic            40  avgt    5   711.988 ± 107.517  ns/op
BenchmarkNoLoader.Posting.Static.postStatic            50  avgt    5  1100.148 ±  58.310  ns/op

Registration

Registration performance hasn't been neglected either...

Benchmark	EventBus v6 NoLoader	EventBus v6 ModLauncher	EventBus v7 (NovaBus)
Dynamic	156007.742 ns/op ± 50069.527	270565.685 ns/op ± 79334.267	136604.073 ns/op ± 32793.549
Static	138840.166 ns/op ± 52563.530	250686.926 ns/op ± 109877.684	121574.491 ns/op ± 57147.494

Due to the way registration is now handled, we can turn off deduplication without needing a memory-heavy huge Deque of
generated unique listeners. This makes the margin of error in these benchmarks much more useful than before:

Benchmark	ClassFactory + Deque	Coordinated duplication
Dynamic	136604.073 ns/op ± 32793.549	108983.740 ns/op ± 28169.456
Static	121574.491 ns/op ± 57147.494	91986.460 ns/op ± 21304.732

Registration data

(click to show/hide)

EventBus v6 benchmark                                     Mode  Cnt       Score        Error  Units
BenchmarkModLauncher.Registering.Dynamic.registerDynamic  avgt    5  270565.685 ±  79334.267  ns/op
BenchmarkModLauncher.Registering.Static.registerStatic    avgt    5  250686.926 ± 109877.684  ns/op

BenchmarkNoLoader.Registering.Dynamic.registerDynamic     avgt    5  156007.742 ± 50069.527   ns/op
BenchmarkNoLoader.Registering.Static.registerStatic       avgt    5  138840.166 ± 52563.530   ns/op

EventBus v7 (NovaBus) benchmark (ClassFactory)         Mode  Cnt       Score       Error  Units
BenchmarkNoLoader.Registering.Dynamic.registerDynamic  avgt    5  136604.073 ± 32793.549  ns/op
BenchmarkNoLoader.Registering.Static.registerStatic    avgt    5  121574.491 ± 57147.494  ns/op

EventBus v7 (NovaBus) benchmark (Coordinated)          Mode  Cnt       Score       Error  Units
BenchmarkNoLoader.Registering.Dynamic.registerDynamic  avgt    5  108983.740 ± 28169.456  ns/op
BenchmarkNoLoader.Registering.Static.registerStatic    avgt    5   91986.460 ± 21304.732  ns/op

Future work

I have further ideas which I've put on hold for now, such as:

• Compile-time transformers powered by NovaLauncher and the new toolchain
  • Some kind of annotation to reduce the boilerplate of EventCharacteristic.SelfPosting(?)
  • Strip empty event listener methods from classes
  • Integration with ValhallaNow! and MR JAR for using value classes before Valhalla is finalised
• Stricter validation of events, both at compile-time and runtime
  • For instance, ensure at compile-time that only records implement RecordEvent
• Direct pass-through of sealed inheritable events that only have one subclass to save memory and reduce startup time

Looking far ahead

To demonstrate the forward-thinking design of NovaBus, here's a look of what might be possible in the far future,
without breaking changes...

This example uses various WIP Java features:

• JEP-401 Valhalla value records
• JEP-468 Derived record creation
• JDK-8303099 Null-Restricted and Nullable Types
• JEP-502 Stable Values

The event would look something like this:

public value record XpChange(Player! player, short amount) implements Cancellable, PlayerXpEvent, ValueEvent {}

And listeners could optionally do something like this:

public static PlayerXpEvent.XpChange? onXpChange(PlayerXpEvent.XpChange! event) {    
    if (event.amount() > 1_000)
        return null; // cancel the event if the amount is too high
    
    return event.with {
        amount += 10;
    };
}

The listener return type is explicitly nullable, indicating it might cancel the event. Returning a null-restricted
PlayerXpEvent.XpChange! would allow NovaBus to treat this as a never-cancelling listener instead. Returning null
cancels the event.

The event.with {} returns a new instance of the event with the changes applied, all other record components left
unchanged. Thanks to Valhalla, this wouldn't necessarily create new instances of the record on the heap on each mutation.
Performance would be as fast as primitives for reading data, with the syntactic sugar of mutable fields for writing data.
Instead of setter methods, validation can be done in the record's canonical constructor.

And that's just the API side of things. Implementation-wise, we could offer the JVM guarantees that the contents of the
built listeners array are never mutated and that they're never null, allowing for much more aggressive optimisations by
the JVM, such as flattening the array's memory layout from pointers of objects with headers into a contiguous block of
direct values.

[Jonathing]

Finally. Documentation.

[LexManos]

Keep

[LexManos]

Looks sane code wise, tons of cleanup needed.
Haven't run the code yet or verified the benchmarks since our last conversation.
But the core of it looks good.

[LexManos]

I think we've discussed this before. The reason for this being counter intuitive is that this optimization takes advantage of the JRE's inlining ability which has a set depth/size budget. It may be worth making note of this because my immediate though was "Why not just unroll into multiple layers if the size matters", but then I remember our conversation and why it wouldn't help.

[PaintNinja]

Good idea, I'll leave a note. In case I forget, the reason why layers (wrapping in batches of 4 manually unrolled methods and chaining those) wouldn't work is because of method inlining.

[LexManos]

Should there be a feature that auto-translates monitor to lowest priority if a event looses its MutableEvent as to not cause breaking changes with compiled code? Probably not important but a passing thought.

[PaintNinja]

Yes, all monitoring listeners have Priority.MONITOR. This is handled in net.minecraftforge.eventbus.internal.EventListenerImpl.MonitoringListener.

All events support monitoring. The MonitorAware characteristic only works with MutableEvent at the moment and is for allowing the event to check if it's in the monitoring phase.

There was the related point brought up on Discord last night about a breaking change for cancellation-aware monitoring listeners (ObjBooleanBiConsumer) going from a CancellableEventBus to a non-cancellable one. This follows the same logic as cancelling listeners - it's assumed that if you're asking about the cancellation status, you care about it and want it to break if it is no longer cancellable. Normal Consumer monitoring listeners would continue working. We need to decide if this is the behaviour we want or if we want monitoring listeners that ask about cancellation status to always get false instead of failing when an event is no longer cancellable.

[LexManos]

So the issue becomes that we will have a crash on the listener's end if an event looses its Cancelability.
Namely the Event.BUS.register() call will crash with either a NoSuchFieldError (If the JVM checks the type of the field) or a ClassCastException/NoSuchMethodError. We should verify what this case is, and see what we can do about making it more obvious to modders what happened.

[PaintNinja]

Yes, it's designed to fail as early as possible now rather than throwing during posting. Best case it won't compile and will tell mod devs the exact line where it's wrong, worst case it'll crash at listener registration time. I think it'll be quite obvious... they'd open up their IDE and see that addListener method call highlighted as not found, so they'd jump to the field definition and see it says EventBus<Foo> instead of CancellableEventBus<Foo>, or they'd look at the available addListener methods and pick a Consumer taking method instead.

Better than compiling and registering fine and only failing on event post imo (v6's behaviour).

[LexManos]

I am fine with the compiler errors.
The runtime issue is the concern, we're targeting mod users at that point. Getting them to the correct place is important.
So at the very least we need a unit test to verify what actually happens in this case. So that we know what the player will see and if its good enough to blame the correct person (mod dev, not us)

[Jonathing]

After spending a not-insignificant amount of time notating the JavaDocs for the API, I feel the need to further express just how impressed I am by all of this. Not only is the API incredibly concise, but even the internal implementations are concise enough -- even though it's kind of hard to read (again, unavoidable considering we're squeezing the JVM for money at this point), it's still clear what each piece of the implementation is meant to do. And this is even more-so for the API.

I think in terms of the API, the only comments I have are to potentially re-assess MutableEvent. I think it was discussed here earlier but since it's only designed to work with MonitorAware, simply calling it MonitorAwareEvent would be enough. That is, unless, there are plans to expand on the capabilities of MutableEvent, but from the looks of it I don't see that happening. Other than that, it's just a matter of cleaning up and making sure everything is as clear as possible to both the maintainers and the consumers. I'm sure Lex has been keeping you company with implementation details anyways.

[Jonathing]

There are a couple of Gradle-related issues with your tests that were reported in the problems file. We need to make sure to account for them.

eventbus-test:test

• The automatic loading of test framework implementation dependencies has been deprecated.
  • This is scheduled to be removed in Gradle 9.0.
  • Solutions
    • Declare the desired test framework directly on the test suite or explicitly declare the test framework implementation dependencies on the test's runtime classpath.
• No test executed. This behavior has been deprecated.
  • This will fail with an error in Gradle 9.0.
  • Solutions
    • There are test sources present but no test was executed. Please check your test configuration.

[PaintNinja]

After spending a not-insignificant amount of time notating the JavaDocs for the API, I feel the need to further express just how impressed I am by all of this. Not only is the API incredibly concise, but even the internal implementations are concise enough -- even though it's kind of hard to read (again, unavoidable considering we're squeezing the JVM for money at this point), it's still clear what each piece of the implementation is meant to do. And this is even more-so for the API.

Thank you, I put a lot of thought into the API design and minimising complexity, I'm glad it's paid off. :)

I think in terms of the API, the only comments I have are to potentially re-assess MutableEvent. I think it was discussed here earlier but since it's only designed to work with MonitorAware, simply calling it MonitorAwareEvent would be enough.

I've explained it here, I think there was a misunderstanding of its purpose that should be solved with better Javadocs in future: #69 (comment)

There are a couple of Gradle-related issues with your tests that were reported in the problems file. We need to make sure to account for them.

These were already broken before the PR, but I've gone ahead and fixed them anyway.

[Jonathing]

Thank you, I put a lot of thought into the API design and minimising complexity, I'm glad it's paid off. :)

Yes! And I hope our recent conversations in voice chat have helped to solidify this. A lot of effort has been put on all three of our parts into making a better understanding of our projects while also focusing scope. I'm very grateful for that.

While it's probably biased based off of my personal experiences with other projects and teams, I think part of the growing pain with this review has been to see how far we can get within scope of EventBus's usage. It's good to, at the bare minimum, try to figure out what we can do within the new EventBus to see if it'd be a reasonable thing to expect of consumers in the future. And when it's not worth it, detailing the reason for that should be given as much care as detailing why a new feature should be added, something I've meticulously done within the PRs I've triaged for Forge itself.

I've explained it here, I think there was a misunderstanding of its purpose that should be solved with better Javadocs in future.

Yes, I agree. I think the JavaDocs will also need to be expanded upon over time as we get more comfortable with the redesign of EB7. Aside from the fact that we (in general, not specifically) are historically bad at naming things, keeping things well documented with use cases, usages, and abilities is going to keep the maintenance of the API in a good state.

[LexManos]

I would like to see more explicit tests. for example, tests verifying that the strict registration throws errors in the cases we want. and that we thimk users will run into. however functionality wise its looking good.

[RealMangorage]

Looks good :D