#Events and EventBus

1 messages · Page 3 of 1

rigid gust
#

and then what happens if you have two types (e.g. source entity type and teleportation type)

#

etc

latent rivet
#

Just make the constructor protected. If someone then makes an anonymous class to fire it anyway, then they're an idiot ¯_(ツ)_/¯

rigid gust
#

yeah that would work I guess

granite nimbus
#

Doesn't solve the whole "unimplementable abstract methods bit"

#

But not a bad solution either

#

I suppose throwing on those methods would work but it also leads to unexpected bugs if a dev doesn't implement them

rigid gust
#

actually it doesn't solve any problem

#

people could still try to listen to the event which is the entire problem

granite nimbus
#

(at least, that's the context XFactHD was replying to)

rigid gust
#

ah, right

latent rivet
#

Another option could be to have the constructor throw on this.getClass() == TheRootEvent.class

granite nimbus
#

The bigger issue is abstract methods in my opinion

latent rivet
#

As far as I can remember without checking, there is not a single event in Forge with an abstract method, all of them just hold the context that makes sense for the given level of inheritance.

granite nimbus
#

Yes but the worry was that mods use it. In the long run, I don't think it's a big deal though - and using abstract classes as "don't listen" is imo far cleaner than an annotation, even if both are suboptimal

honest timber
#

Yeah it’s fields can be identity classes, e.g. optional will become a value class

#

(If that’s what you’re referring to)

rigid gust
#

I just realized that we can use methodhandle combinators to place the cancellation checks etc inside the classes that LMF generates

primal mist
# honest timber Yeah it’s fields can be identity classes, e.g. optional will become a value clas...

no the question is if you can pass a value type as an argument, have the function modify a field inside the value type, and have that show up in the calling function [via passing the arguument as a pointer to the calling function's stack]. In C# you have to use an explicit "ref" parameter for this.

though the relevancy to events is tangential, since i am skeptical that events would benefit much from being value types even if this works.

honest timber
# primal mist no the question is if you can pass a value type as an argument, have the functio...

Nah that’s not supported as far as I’m aware, they’re completely immutable and I don’t believe there’s any concept of pointers. Value types would only really be useful for fire and forget events which is not many of them haha.

Unless you stored the current fired event somewhere and allowed event handlers to update that and then the updated version is returned to the event firer

inland dawn
#

Imagine that at the end of each level, there is a bus that takes you to the next level.

wheat sinew
#

and that bus runs on an extremely tight schedule

rigid gust
outer steeple
#

if you're touching those it's good to clean it up as well

rigid gust
#

true, true

#

especially since some form of inheritance is probably here to stay

#

changes made

rigid gust
#

@outer steeple pong

rigid gust
#

methodhandles aren't even that slow

#

on a test with 100 listeners and an event firing 10 times:

Benchmark                         Mode  Cnt   Score   Error  Units
ComparisonBenchmark.testCl        avgt   30  11.535 ▒ 0.689  us/op
ComparisonBenchmark.testCl:Àjfr   avgt          NaN            ---
ComparisonBenchmark.testLmf       avgt   30  13.517 ▒ 0.719  us/op
ComparisonBenchmark.testLmf:Àjfr  avgt          NaN            ---
ComparisonBenchmark.testMh        avgt   30  19.878 ▒ 2.209  us/op
ComparisonBenchmark.testMh:Àjfr   avgt          NaN            ---
rigid gust
#

@outer steeple new PR just dropped 😛

outer steeple
#

can you run a benchmark too

rigid gust
# 
Benchmark                                            Mode  Cnt   Score   Error  Units
EventBusBenchmark.testClassLoaderCombined            avgt   15  71.410 ▒ 7.078  ns/op
EventBusBenchmark.testClassLoaderDynamic             avgt   15  30.531 ▒ 2.803  ns/op
EventBusBenchmark.testClassLoaderLambda              avgt   15  27.355 ▒ 2.896  ns/op
EventBusBenchmark.testClassLoaderStatic              avgt   15  35.135 ▒ 5.245  ns/op
EventBusBenchmark.testModLauncherCombined            avgt   15  71.563 ▒ 4.830  ns/op
EventBusBenchmark.testModLauncherDynamic             avgt   15  31.274 ▒ 2.253  ns/op
EventBusBenchmark.testModLauncherLambda              avgt   15  27.076 ▒ 2.112  ns/op
EventBusBenchmark.testModLauncherStatic              avgt   15  28.678 ▒ 3.190  ns/op
EventBusBenchmarkNoLoader.testNoLoaderCombined       avgt   15  74.308 ▒ 6.854  ns/op
EventBusBenchmarkNoLoader.testNoLoaderDynamic        avgt   15  27.666 ▒ 3.103  ns/op
EventBusBenchmarkNoLoader.testNoLoaderLambda         avgt   15  27.320 ▒ 2.308  ns/op
EventBusBenchmarkNoLoader.testNoLoaderStatic         avgt   15  28.644 ▒ 2.260  ns/op
#

note that this benchmark is not good at measuring the impact of this, there's not enough listeners

rigid gust
#

getting close to finishing the TODO list now...

spice sphinx
#

did you already stab cancellable?

rigid gust
#

No I think I'm gonna keep it for now

spice sphinx
#

bad

rigid gust
#

I guess now would be the time

#

But I'd have to change the signature of post()

agile slate
#

Would you?

rigid gust
#

well, not necessarily

#

but ideally you'd only get a boolean result if the event is cancelable in the first place 😉

spice sphinx
#

just remove the annotation and make cancellable events override setCancelled

#

pretty much all the annotation does anyway

rigid gust
#

what would be a good name for "post a cancelable event and return true if the event was canceled"?

rigid gust
spice sphinx
#

honestly fuck that whole contract, it should be false if cancelled and true if successful

#

every call to it is !post(blah)

rigid gust
#

not necessarily

#

if (post) return;

#

and changing that is too much of a pain to port to for no benefit at the end of the day

#

I'd just like to find a better method name

spice sphinx
#

guess what you're changing the method signature, so the effort is the same™️

rigid gust
#

maybe postCheckCanceled

spice sphinx
#

and then you can just name it postCancellable

rigid gust
#

that doesn't say anything about the return value

#

would be nice if one could tell just from the callsite what it means

spice sphinx
#

because it doesn't need to compensate in the name for its shitty inverse contract

rigid gust
#

(also it's cancelable :P)

spice sphinx
#

The word cancellable (which is also but less commonly spelled cancelable)

#

wrongo™️

rigid gust
#

(US usually cancelable)

spice sphinx
#

the eventbus has been not only wrong but used both names for a while

rigid gust
#

double l is only used in comments stabolb

#

anyway

rigid gust
#

something like postIsCanceled makes more sense

#

or postAndCanceled

#

wait hear me out

#

what if the event itself was returned after post

#

so you could do post(...).isCanceled()

#

how did I not think of this, it's so obvious

#

maty get reviewing so I can PR this! stabolb

#

or if someone else wants to review my 2 PRs... be my guest

spice sphinx
#

To be fair, the implicit contract of java lib collections that return a boolean status (such as List#add) is to return if it was successful and not add any verbosity to the name

#

C# usually prepends Try to functions that return a status (but java doesn't, so that's mostly irrelevant here)

rigid gust
#

returning the event is the most flexible approach

primal mist
#

a lot of event factory methods do "return null if canceled otherwise either the event or some result property of the event"

spice sphinx
#

Ultimately post returning the event is probably pointless

#

because its just going to transform

# 
var event = new MyEvent(...);
post(event)
if(event.isCancelled() { ...

to

var event = new MyEvent(...);
if(post(event).isCancelled()) { ...
#

honestly the former is probably preferrable

primal mist
#

if only Java had the comma operator

#

if (post(event), event.isCanceled())

#

hmmmmmmm... thonk

#

aww you can't do post(var event = new MyEvent(...))

wheat sinew
#

is if ((event = bus.post(new MyEvent())).isCancelled()) valid

latent rivet
#

Yes

spice sphinx
#

only if event is pre-defined, yes

#

so you aren't really saving lines

wheat sinew
#

what about if (bus.post(new MyEvent()) instanceof SomeTypeIDK event && event.isCancelled())

gusty dove
#

Thats.... clever

wheat sinew
spice sphinx
#

stab

rigid gust
spice sphinx
#

That looks terrible when you have params and/or need to retain the event

#

may as well make post be void

rigid gust
#

I can also keep the current signature even if the event is not cancelable

#

nobody says you have to use that signature, however it is nice to keep things one-liners

#

you could also do var event = post(new MyEvent()); return event.isCanceled() ? null : event

spice sphinx
#

Why would you ever want that

#

that just obscures .isCancelled by a later != null check

rigid gust
#

Well I thought ForgeEventFactory did that sometimes?

rigid gust
#

I want to make cancelable events an interface that people can implement on their events instead of an annotation:

public interface CancelableEvent {
    default void setCanceled(boolean canceled) { ((Event) this).canceled = canceled; }
    default boolean isCanceled() { return ((Event) this).canceled; }
}

public class Event {
    /* package-private */ boolean canceled = false;
    
    // ...
}

this will remove the autocomplete of methods that shouldn't be called.

Now, my question for <@&1128776937410670663>s is:
What should the return value of EventBus#post be?

Here are a few options:

  1. Keep current status quo: true if event is cancelable and was canceled.
  2. Flip the boolean return to mean false if the event was canceled.
  3. Make post return the event itself so that people can chain an .isCanceled() call if they want.
  4. Make post return void.
  5. Make post return void and add a new postCancelable overload that returns a boolean, the overload only being available for <T extends Event & CancelableEvent>, of course.

What do you think?

#

I really like 3. myself

humble junco
#

I'm divided between 3 and 5

#

would postCancellable do anything at all besides return cancel status?

rigid gust
#

and you still couldn't tell just from the name of the function whether it is true or false that stands for canceled

humble junco
#

then 3. IMO

spice sphinx
#

Its probably a legacy reason for not writing the event classname in the patch files

inland imp
#

At least implementation wise

rigid gust
#

why?

inland imp
#

The cast ((Event) this) is very very very expensive

rigid gust
#

it's only used by mods though

#

the eventbus will use a static final VarHandle internally to read canceled

inland imp
#

It is not about the read

#

The set is also a problem

rigid gust
#

yes but it's not called nearly enough for it to matter

inland imp
#

You do not know that!

#

Be carefull in these kinds of libraries

#

Modders use events and their features in interesting ways

#

Some fire multiple events per frame

#

Forge even has some cancellable events which are fired multiple times per frame

#

I have nothing against the concept of a CancellableEvent

humble junco
#

could we have a CancellableEvent subclass from Event instead of the interface?

rigid gust
humble junco
#

CancellableEventBase implements CancellableEvent then

rigid gust
#

I am not buying the FUD argument, Orion, events only get canceled once 😛

humble junco
#

if someone wants to skip the base class they can declare their own field

inland imp
#

Please for the love of god, we have been trying to get rid of this pattern every where in all client components

#

Do not add it here

#

It is not a good pattern

#

And terrible for performance

rigid gust
#

it leads to a much nicer API

#

and I'll bet you it has ZERO impact on performance in practice!

inland imp
#

It does not

humble junco
#

also how does CancellableEvent extends Event mess up the API?

inland imp
#

We tested this to death a year ago

latent rivet
inland imp
#

Yes

rigid gust
#

fine I'll write a JMH profile

humble junco
#

wait nevermind, I answered that myself

inland imp
#

Me, @rustic kelp and @manic void ran several tests

#

We discovered this because we used this exact pattern in extension classes

#

And could not figure out why the new bakedmodel code was got awefully slow

#

Well turns out

#

The bytecode that gets generated from that construct is horrible

outer steeple
#

done

inland imp
#

That could work maty

#

As long as it does not do the unknown cast

#

You are fine

rigid gust
#

yeah that would work

#

let's benchmark the cast though

inland imp
#

Be my guest

#

I am 99,99% sure that it is about 100 times slower

spice sphinx
#

The varhandle impl sounds like a hacky mess, just leave all the cancellable code on Event and make cancellable events override setCancelled to flip a protected isCancelled field

humble junco
#

isn't the point to not have public setCancelled methods on events that are not cancellable?

inland imp
#

Yeah

#

@rigid gust What you can do is put the event and the interface in the same class

#

Then add a Event getThis() method to the interface

#

And then access a package private field

rigid gust
#

why same class?

#

same package is fine

#

not very scientific, but still

Benchmark                         Mode  Cnt   Score    Error  Units
CastBenchmark.testDirect          avgt   15  55.673 ▒ 17.477  ns/op
CastBenchmark.testDirect:Àjfr     avgt          NaN             ---
CastBenchmark.testInterface       avgt   15  47.756 ▒  3.776  ns/op
CastBenchmark.testInterface:Àjfr  avgt          NaN             ---
#

error on the first one is too high

#

but it is nowhere near that 100x you mentioned and in fact looks quite fine

# 
package net.neoforged.bus.benchmarks;

import net.neoforged.bus.api.BusBuilder;
import net.neoforged.bus.api.Event;
import net.neoforged.bus.api.IEventBus;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.infra.Blackhole;

@State(Scope.Benchmark)
public class CastBenchmark {
    public static class TestEvent1 extends Event implements SomeInterface {
        private int value = 0;
    }

    public static class TestEvent2 extends Event {
        private int value = 0;

        public int getValue() {
            return value;
        }

        public void incrementValue() {
            value++;
        }
    }

    public interface SomeInterface {
        default int getValue() {
            return ((TestEvent1) this).value;
        }
        default void incrementValue() {
            ((TestEvent1) this).value++;
        }
    }

    private final IEventBus bus = BusBuilder.builder().build();

    @Setup
    public void setup() {
        for (int i = 0; i < 20; ++i) {
            int finalI = i;

            bus.addListener(TestEvent1.class, event -> {
                if (event.getValue() < finalI) {
                    event.incrementValue();
                }
            });
            bus.addListener(TestEvent2.class, event -> {
                if (event.getValue() < finalI) {
                    event.incrementValue();
                }
            });
        }
    }

    @Benchmark
    public void testInterface(Blackhole blackhole) {
        TestEvent1 event = new TestEvent1();
        bus.post(event);
        blackhole.consume(event);
    }

    @Benchmark
    public void testDirect(Blackhole blackhole) {
        TestEvent2 event = new TestEvent2();
        bus.post(event);
        blackhole.consume(event);
    }
}
latent rivet
#

Is the field supposed to be the canceled flag?

humble junco
#

in event. same thing TEch is suggesting but instead of casting, use a getThis() that Event implements

latent rivet
#

Yeah, I realized that after sending 😅

#

AFAICT, the getThis() method would have to be implemented by every single event implementing that interface though. I don't see how that's better than a CancellableEvent extends Event class. But maybe I'm missing something really obvious here

humble junco
#

hmmm... think3d ```java
protected interface AnEvent {
Event self();
}

public interface CancellableEvent extends AnEvent {
default void setCancelled(boolean cancelled) { self().isCancelled = true; }
}

public class Event implements AnEvent {
/* package private */ boolean isCancelled;

public Event self() { return this; }

}

public class SomeCancellableEvent extends SomeEvent implements CancellableEvent {
/* nothing special to do here */
}

humble junco
latent rivet
#

Makes sense, yeah

rigid gust
#

I mean, you are overthinking this

humble junco
#

oww can't make protected interfaces :P

rigid gust
#

I proved that my original design doesn't have a substantial impact on performance, feel free to copy/paste the benchmark and prove me wrong 😛

humble junco
#

I mean

#

to me it isn't about performance

#

it's about self() being much nicer to look at than casting

rigid gust
#

For 2 methods it doesn't make any difference

#

here is another benchmark which had lower variance:

Benchmark                         Mode  Cnt   Score   Error  Units
CastBenchmark.testDirect          avgt   15  48.783 ▒ 4.505  ns/op
CastBenchmark.testDirect:Àjfr     avgt          NaN            ---
CastBenchmark.testInterface       avgt   15  47.159 ▒ 3.426  ns/op
CastBenchmark.testInterface:Àjfr  avgt          NaN            ---
humble junco
#

what's the impl of the test, out of curiosity?

agile slate
#

could the difference between your results and orions be the java version?

rigid gust
rigid gust
humble junco
#

ah so it's just one post call per loop

#

maybe 15 counts is too small for a meaningful result?

rigid gust
#

each count corresponds to 2 seconds of continuous runs

humble junco
#

oh nevermind, thought it counted individual executions of the test code

inland imp
#

@rigid gust Show me the code

#

We ran this on 1.16.4

humble junco
#

he pasted it above

#

it's possible 1.17 has faster casting?

#

err java17

#

vs java8

rigid gust
inland imp
#

I don't know

#

Might be that the JVM does different inference at runtime

gusty dove
#

@rigid gust i get a feeling the jvm will devirtualize SomeInterface in that test

inland imp
#

Yeah

gusty dove
#

because there is only one class implementing it

inland imp
#

I was thinking that

#

And also, because the callsite to the increment is on the concrete type

#

There might be hot code path detection in the jvm that then unfolds it

#

@manic void This is more your area of expertise

gusty dove
#

jvm is annoying like this

inland imp
#

Do you remember what triggered that stupid casting performance issues during our rendering refactor tests?

outer steeple
#

who wants to ask jas too

gusty dove
#

i think she's quite busy atm

rigid gust
#

so I really don't know how this happened to you

inland imp
#

We literally had these tests

#

I can't find them at the moment

#

And we could prove it too

#

Because the bytecode in the JVM is garbage

#

It does a bunch of additional checks

#

To make sure that the cast is safe

#

In those virtual calls

queen stratus
#

Is a potential solution having isCancelled in the Event class, having it always return false, then having an abstract CancellableEvent to extend where the setCancelled exists and overrides the isCancelled check? No casting required then?

spice sphinx
#

I mean yeah checkcast isn't free, and you have the problem of devirtualization if there are few impls

inland imp
#

Yeah

#

we did this with many different implementations of IForgeBakedModel

#

Which was our case

#

That had such a cast to get self

#

And then fallback to the vanilla call if the forge custom method was not overriden

spice sphinx
#

Hold on, can we just add setCancelled as protected and make cancellable events override to public and call super?

rigid gust
#

checkcast isn't free but it should be cheap enough here, especially since setCanceled is only called once per event at most (usually)

inland imp
#

That is the point

spice sphinx
#

once per event for an event(s) fired per frame

inland imp
#

Because the JVM has no idea about the type hierarchy the checkcast is far from free

spice sphinx
#

protected with an "override to public" idiom would work

rigid gust
#

did another more elaborate test:

Benchmark                         Mode  Cnt    Score    Error  Units
CastBenchmark.testDirect          avgt   15   96.241 ▒ 11.749  ns/op
CastBenchmark.testDirect:Àjfr     avgt           NaN             ---
CastBenchmark.testInterface       avgt   15  107.663 ▒ 21.949  ns/op
CastBenchmark.testInterface:Àjfr  avgt           NaN             ---

(source code: https://gist.github.com/Technici4n/a4709f37b90c85e6fab85a1e0d7c8dc1)

yet again there is no visible difference

rigid gust
inland imp
#

Can you push your code

rigid gust
inland imp
#

I will check this out later

rigid gust
#

gradlew jmh to run it should do, otherwise lmk

#

don't get me wrong I think we should be careful with performance, but we should also understand that events that fire once per frame are not a problem per se

#

atm posting a non-cancelable event seems to be about 10 + 2*listenerCount nanoseconds

queen stratus
#

This issue only exists if you make it an interface

rigid gust
#

my claim is that it's not an issue and I want this settled once and for all!

queen stratus
#

But casting is going to be less performant than not

rigid gust
#

it does not make a sufficient difference

queen stratus
#

But there is a difference

#

So why would you insist on it being an interface when you could go with the other solution and not have the performance loss because you don't need to cast

rigid gust
#
  1. making it a class or adding artificial methods leads to a worse API
  2. I want to stop this argument once and for all
queen stratus
#

How does it make it worse API when to the modder it looks the same

rigid gust
#

abstract superclass is not an option

#

because something like PlayerEvent might have cancelable and non-cancelable subclasses

inland imp
#

We can table this for now

#

I will dig up the code tomorrow

queen stratus
#

That's a reasonable explanation - one potential issue with an interface though is you'd have to implement canceled/setCanceled in every class that implements the interface, no?

inland imp
#

And then check it out

rigid gust
#

(note that we can easily change the casts to a varhandle lookup if we want to in the future, so checking this is not urgent)

rigid gust
granite nimbus
#

And yeah, I'd say an Event getThis() method is probably the most sensible, honestly

granite nimbus
#

I guess for storing the cancellable field, right... that whole system seems iffy to me honestly

#

I have some ideas for how else you'd do it but they'd all be pretty intrusive redesigns which I already know people here are against

spice sphinx
#

Idk why protected method that gets overridden to public is not a viable option

granite nimbus
#

It's a viable option, there's just other viable options that it doesn't really offer advantages over

spice sphinx
#

right now we just jam it in the class doc

#

which is bad for organization

wheat sinew
#

oh hey, the self-cast thing was brought up again thinkies

spice sphinx
# 
/**
* Cancelling this event does xyz
*/
public void setCancelled(boolean cancel) {
...
granite nimbus
spice sphinx
#

Implementing getThis is churn

#

Overriding setCancelled, which must be done to document it, is implicit

granite nimbus
wheat sinew
#

iirc covers, ama, and Orion observed through tests that a self-cast has a non-negligible penalty, and that it was recommended that any such self-cast in rendering extension interfaces should be limited to at most once (i.e. in something like IForgeBakedModel, any method which needs the BakedModel more than once should start with a BakedModel model = self() and use that variable onwards)

#

I think the basis they used for the test was for IForgeTransformation

granite nimbus
spice sphinx
#

That is also fine

granite nimbus
#

And the events are responsible for grabbing from or shoving to the field in it

spice sphinx
#

Though the event bus has to know the concept of cancellation

granite nimbus
#

Yeah, protected field

spice sphinx
#

to not propagate to further listeners

#

so isCancelled may as well be on Event

granite nimbus
#

Hmm. The whole system seems iffy. I would refer you to this: #1136448404495532133 message

#

But yeah, the whole "cancellable field on event when not all events are cancellable" kinda smells to me

#

But no easy way around it without more intrusive changes

spice sphinx
#

Well it doesn't have to be, right
but then you have to instanceof Cancellable to check if propogation should halt

rigid gust
#

What's going on

granite nimbus
#

Hell, that would speed up non cancellable events as well I would think

spice sphinx
#

Maybe? I'm not super versed in that part of the internals

granite nimbus
# rigid gust What's going on

Talking about the smelliness of having a cancellable field in the base event class, and about how maybe making events override the cancellable methods isn't a bad thing so that they're documented

spice sphinx
#

Tech is that a thing we can do thonk

granite nimbus
#

Which I believe has happened?

rigid gust
#

I already made that change

#

Inheritance is kept

granite nimbus
#

Listener inheritance is what would hold it up before

#

Well, sorta

#

Actually maybe not

#

Yeah, on second thought that should be possible

rigid gust
#

But there is no isCanceled filter anymore for non-cancelable events

spice sphinx
#

A non-cancellable event shouldn't subclass a cancellable event

granite nimbus
rigid gust
#

I don't think anything suggested here is better than my original proposal

granite nimbus
#

Just yeeting the cancellable field entirely that is and using the interface directly

granite nimbus
viscid mangoBOT
#
lukebemish

[Reference ➤ ](#1136448404495532133 message)Talking about the smelliness of having a cancellable field in the base event class, and about how maybe making events override the cancellable methods isn't a bad thing so that they're documented

spice sphinx
#

You can keep your interface but it doesn't need a default impl

granite nimbus
#

That's why it's better

#

Still an interface, yeah

#

No defaults, and no field in event

spice sphinx
#

The method must be overridden in the target to describe the effects of calling it

rigid gust
#

Well then people need to implement the methods themselves

spice sphinx
#

sucks, its super trivial to do

rigid gust
spice sphinx
#

It is not appropriate for the class doc

rigid gust
#

You're not solving any problem

granite nimbus
#

The field in event is really smelly

spice sphinx
#

It is an effect of calling setCancelled

rigid gust
spice sphinx
#

The class doc has been packed with all sorts of garbage because of the annotations, not because it is the proper place for it

granite nimbus
#

Yeah, the effects of cancellation should go in the docs for the methods involving cancellation

#

Makes the docs way easier to read

rigid gust
#

Meh reading 3 lines of class javadoc won't kill you

granite nimbus
spice sphinx
granite nimbus
spice sphinx
#

its just "somewhere" in the class doc

rigid gust
#

That's good enough

rigid gust
granite nimbus
#

Neither is propagating code smell, and that's what the field in event is

rigid gust
#

I don't care about code smell if it doesn't have an impact on modders

granite nimbus
#

It also means that, tech, with your system - an event could implement CancellableEvent, override the impl of isCancelled, and not see any change in what EventBus considers cancelled

rigid gust
#

Indeed, that's why the method is annotated with @NonExtendable

granite nimbus
#

If you're using what are supposed to be effectively final methods... then an interface isn't the right place for them...

#

The whole thing is ugly. If implementing the interface makes something cancellable... then it should actually do that, not just enable built in functionality

#

Additionally, is there any reason not to let people implement their own cancellable implementations?

rigid gust
#

The entire concept of cancelation gets special treatment from the eventbus

granite nimbus
#

After all, the existence of that interface is what determines whether to enable all the cancellable stuff, right?

rigid gust
#

Due to inheritance it is somewhat more complicated than that

granite nimbus
#

How so? You're constructing a list of listeners for an event type

rigid gust
#

The listener has to anticipate for cancelable subclasses

granite nimbus
#

Inheritance should only come in in terms of which listeners get shoved in the array

granite nimbus
rigid gust
#

They do, but currently the code works by removing the check, not adding it

granite nimbus
#

How does it remove the check exactly?

rigid gust
#

Otherwise the receiveCanceled boolean has to be kept somewhere

#

Anyway I'm off to bed. Remember that goal #1 is to keep the code close to the current version and not add boilerplate, everything else comes after that

granite nimbus
#

Looking over that it seems like you're just filtering stuff down one of two different paths based on the event type. The extra filter information you keep around could be kept around in the outer class, and just not used, but used by the inner class. That all seems doable to a degree

granite nimbus
spice sphinx
#

That's a shit goal lol

manic void
wheat sinew
#

ye, that's what I remember

#

i wonder if we should make an issue on the NF repo to track that

#

just so we remember

outer steeple
#

even though the tests don't seem to show a meaningful difference, the cancelableevent interface can declare an abstract Event getThis() and Event can soft-override it

spice sphinx
#

yes but that's just stupid, we can instead just make the events override setCancelled

tranquil totem
#

Why was cancellable powered by asm anyway?

spice sphinx
#

because annotations™️

tranquil totem
#

Was there a reason or did lex just do it that way?

outer steeple
#

and in any case don't forget that isCancelled is a thing too

#

it's not just one method you have to implement

humble junco
humble junco
primal mist
#

other way around

humble junco
#

oh right brainfart. the issue is making the parent public, without the children

#

I still don't like the pattern either way cos overriding setCancelled isn't a good visual indicator , but I guess it would work

primal mist
#

it's also a compile time only problem, i'm not sure what the override structure being considered is

#

what if Event were an interface and CancelableEvent extended it?

humble junco
#

then everyone needs to impl the field, get and set

#

lots of duplication

#

not an issue just annoying

primal mist
#

EventBus.cancelCurrentEvent

spice sphinx
rigid gust
#

I'm not gonna add boilerplate for cancelable events, so we can probably stop this discussion here and move on to more productive things

granite nimbus
#

I'm sorry but why is your opinion suddenly the one that ends discussions? This is a worthwhile discussion. Even if you don't think it's a good approach, discussing it means that someone else who does think it's good could go implement it themselves

rigid gust
#

We can continue the discussion if you accept the obvious requirement that code should not be made a lot more verbose for no gain

spice sphinx
#

mfw adding

@Override
public void setCancelled(boolean cancel) {
    this.cancelled = true;
}

is "a lot more verbose"

rigid gust
#

And you need a getter too

spice sphinx
#

"oh no"

#

its like 3 more lines

#

+1 for the field

#

and that's if all of it gets packed into the children

rigid gust
#

Yet that doesn't bring any benefit to modders?

spice sphinx
#

if it stays protected in Event then only setCancelled is needed

#

The benfit is having the documentation of cancelling an event on the relevant methods, not jammed in the class doc where it does not belong

#

the overhead is exceptionally minimal

rigid gust
#

Lol if that's your only reasoning I am sorry to tell you that it wont fly

spice sphinx
#

You don't get to make the call :p

#

if we're stuck disagreeing we have to call a full vote

agile slate
spice sphinx
#

and we are already taking changes that necessitate updating nearly every event

rigid gust
#

If you're gonna call a full vote because putting documentation in the class is too much of an ask then wtf

spice sphinx
#

Removing the annotations means every cancellable event has to be touched already

rigid gust
#

Yes and it's a trivial replacement by an interface

#

Not having to copy-paste boilerplate into each event

agile slate
spice sphinx
#

and? Then they are never isCancelled, it's not a huge issue. The listener list is going to query cancelled status anyway to propagate listeners

inland imp
#

I just woke up, and am going to do some number running later

#

If I can't figure it out we can take what tech proposed and call it a day

rigid gust
#

Orion it might have been the non-private self() method (java 8 didn't have private interface methods)

rigid gust
#

Shadows I would like you to understand that it is extremely annoying to have people come and bikeshed a single API element and then threaten request a full vote when I tell you that no that design is not practical

spice sphinx
#

and I'm not in the mood for keeping stuff in a legacy spot "just because"

#

it is only there because the system was annotation-based

rigid gust
#

If legacy means less boilerplate then so be it

#

Your only argument is that class javadoc is apparently not suitable for documentation on how the event behaves when canceled, please dude

agile slate
#

shadows the easiest solution if we don't have a consensus here is to leave it as it is ... annotation based

wheat sinew
#

retain the status quo if no consensus to change

#

we can always revisit this discussion later down the road, perhaps when someone makes a proof of concept

inland imp
granite nimbus
rigid gust
#

then what is the gain for modders?

granite nimbus
#

But you shouldn't be acting like your opinion is somehow authoritative here

rigid gust
#

(well, it should be since I am the one doing the refactor)

inland imp
#

His opinion is what matters the most, since he is writing it

granite nimbus
inland imp
#

When is done, it needs to be reviewed

#

And then his opinion is put up against other moderators

granite nimbus
inland imp
#

Ah okey

#

Ignore me then

granite nimbus
#

I'm talking about Tech shutting down discussion of alternatives. Not about him not implementing alternatives

inland imp
#

Understood

granite nimbus
#

Obviously he doesn't have to implement squat

inland imp
#

Misread the statement then

granite nimbus
#

But saying "i won't write that so let's not talk about it" is... just not good

rigid gust
#

I strongly doubt we'll get to a better alternative but I digress

inland imp
#

Personal opinion: I dislike the default implementation on the interface.........

agile slate
rigid gust
inland imp
#

However I think we need some kind of common logic

#

IMHO from the event bus's perspective, a cancellable event, only needs one property:

public interface CancellableEvent {

    boolean isCancelled();
}
granite nimbus
inland imp
#

How the cancel is triggered is really not relevant for the event bus

granite nimbus
#

That, and yeeting the cancelled field on event to go with that

rigid gust
#

if we remove the field, it has to be re-added by every single cancelable event...

wheat sinew
#

i think the idea behind that documentation point might be similar to why the JDK introduced the SequencedCollection interfaces in J21
having that interface -- or in this case, having a declared setCancelled method -- is more powerful in pointing consumers about a property of this collection/event, rather than being relegated simply to a doc comment

inland imp
#

Yeah I understand tech

#

But that can be solved differently.

#

Like it is technically a different discussion

#

From the event busses perspective

#

It does not matter

#

Whether it is a field

#

Or something else

#

For us as implementers

#

It matters

#

Because it means less work

rigid gust
#

well, virtual dispatch is not free either

inland imp
#

Sadly no goorvy Trait here

inland imp
#

VDispatch is much cheaper then check cast....

#

And that had no impact either in your tests

spice sphinx
#

jvm virtual dispatch is quite fast

rigid gust
#

well idk, removing the isCanceled check for non-cancelable events made a difference when I made a proper benchmark some time ago

inland imp
#

Yeah for sure, because it is a useless call

#

So the trade off there is for sure worth it

#

But IMHO the API design of the event is much nicer with it going through the method

#

Instead of straight for the field

outer steeple
inland imp
#

Yeah

rigid gust
#

yes

#

back to our problem, I am willing to compromise a bit on the design on the eventbus side if it makes the API nicer for users

spice sphinx
#

This is really in the same vein as the result stuff

Documentation should be organized where it is relevant

rigid gust
#

the problem with the result is that it's a generic enum that is being misused

spice sphinx
#

There are three "weird" things in event class docs: results, cancellation, and the target event bus

#

the first two we can move

#

the latter is... weird af and has no home

rigid gust
#

if you get the bus wrong your listener won't register

#

so there is some level of protection

#

same level as calling setCanceled on an event without the @Cancelable annotation

#

(i.e. you get a runtime check)

spice sphinx
#

I mean that's a reasonable level of protection but I wish it was a bit better

#

I still sorta think the multi-bus concept is weird

rigid gust
#

it is but it will allow us to make events parallel in a nice way

spice sphinx
#

Maybe we should just hide the type of bus from the modders

#

i.e. make the api just "add listener"

rigid gust
#

How can we tell which bus?

granite nimbus
#

The event class

spice sphinx
#

Same way we do the runtime failure check, lol

rigid gust
#

Virtual Dispatch (i.e. ((CancelableEvent) event).isCanceled())

Benchmark                                   Mode  Cnt    Score    Error  Units
VirtualCallBenchmark.testCanCancel          avgt   15  183.083 ▒ 11.032  ns/op
VirtualCallBenchmark.testCanCancel:Àjfr     avgt           NaN             ---
VirtualCallBenchmark.testCannotCancel       avgt   15   81.196 ▒  4.194  ns/op
VirtualCallBenchmark.testCannotCancel:Àjfr  avgt           NaN             ---

VarHandle access to isCanceled

Benchmark                                   Mode  Cnt    Score   Error  Units
VirtualCallBenchmark.testCanCancel          avgt   15  171.539 ▒ 5.807  ns/op
VirtualCallBenchmark.testCanCancel:Àjfr     avgt           NaN            ---
VirtualCallBenchmark.testCannotCancel       avgt   15   82.577 ▒ 5.970  ns/op
VirtualCallBenchmark.testCannotCancel:Àjfr  avgt           NaN            ---
#

so no real difference

#

@inland imp btw

inland imp
#

Meh

#

You know what

#

I trust what ever you come up with

#

Sorry to have wasted your time

rigid gust
#

lol don't worry, you did not

#

this means that we can let people override isCanceled if they want to

inland imp
#

I would say that is a great idea

#

Personally I would not even bother with the field in event at all, at least not in the base library

#

And maybe make some sort of ForgeBaseEvent

#

that holds that magic

rigid gust
#

so basically abstract BaseCancelableEvent?

inland imp
#

No

rigid gust
#

the problem is that e.g. some PlayerEvent subclasses need to be cancelable and some others not

inland imp
#

What I am saying is: have the event library not care for how cancellation is done

#

But then in forge create your system

granite nimbus
inland imp
#

Potentially with some ASM processing to deal with the downsides

granite nimbus
#

Just use the interface

inland imp
#

That way, you have a very clean API in Bus itself

rigid gust
#

I'm trying to remove ASM, not add more of it 😛

inland imp
#

And deal with the ugly side of things, in a place were you have many many many more tools available

#

I am not saying you need to use ASM

#

I am saying move that ugly implementation detail to a place were your toolbox is much bigger

#

And were you have many more possibilities

rigid gust
#

due to single inheritance a base class approach doesn't seem to work

spice sphinx
#

keeping it protected in Event works :p

rigid gust
#

that doesn't remove the boilerplate of needing to override two methods?

spice sphinx
#

I still think you can leave isCancelled public, only the field and the setter need be protected

#

hell, the field can be private

#

the setter just needs to call super

rigid gust
#

it shouldn't be left public otherwise it is too easy to think your event is cancelable when it is not... :/

spice sphinx
#

Is it? I wager most people never check isCancellable unless they are explicitly using a receiveCancelled listener

rigid gust
#

well the event author checks it at the very least

spice sphinx
#

The author isn't going to accidentially forget their event is cancellable (or the inverse)

rigid gust
#

it can happen

#

I'm trying to avoid cancellation-related methods in non-cancelable events

#

if we can't achieve that then we'll keep the annotation

inland imp
#

And forego any default implementation anywhere

rigid gust
#

I know, but it means that we'll need to copy/paste the cancellation boilerplate in most PlayerEvent subclasses... unless we also make a CancelablePlayerEvent class lol 🤨

inland imp
#

Yeah

#

That is the problem you are in

#

Without a trait system like groovy there is no other clean option

#

You will always need to muddle the water

#

Somewhere

rigid gust
#

yeah

inland imp
#

That is the beauty of the ASM solution XD

#

It fixes that mess

#

Cause it injects the "trait" afterwards 😄

rigid gust
#

but I removed the subclass transformer, don't make me add it back xD

inland imp
#

Xd

#

I don't have a solution for you man

rigid gust
#

yeah I don't think one exists

inland imp
#

I have the luxury in ngnext when such a problem came up that Mary wrote me a whole groovy Ast processor that generates the implementations for me

#

Would a annotation processor be able to do this?

#

Like you add the interface it detects it

#

And then generates the field and implementations of the methods for you?

#

At compile time?

rigid gust
#

well ideally we don't need post-processing of the jars anymore

inland imp
#

Aps are fine

#

They are part of the jdk/eclipse compiler and supported by all of the frameworks we use

#

However I think an AP needs a custom annotation to make it work

#

But that would be trivial

rigid gust
#

I mean, that is a lot more complicated than putting a canceled field in Event

inland imp
#

However I don't think it can modify the original class.....

inland imp
#

Then again I do not think the AP can be used after all

#

Since it can't modify a class

#

Only generate new ones

rigid gust
#

code transformations should be last-resort imo

inland imp
#

Not really...

#

This is compile time processing

#

Which in my opinion is fare game

#

But given that you can not modify the existing class

#

I think it them becomes more difficult

spice sphinx
charred sluice
#

Java annotation processors don't always play nicely with other jvm languages

rigid gust
# spice sphinx Ultimately having only part of the methods be available (the getter) is preferra...

I stand by

/**
 * Interface for events that can be canceled.
 * Listeners registered to the event bus will not receive {@link #isCanceled() canceled} events,
 * unless they were registered with {@code receiveCanceled = true}.
 */
public interface CancelableEvent {
    /**
     * Sets the cancel state of this event.
     *
     * <p>This will prevent other listeners from receiving this event unless they were registered with
     * {@code receiveCanceled = true}.
     * Further effects of setting the cancel state are defined on a per-event basis.
     *
     * <p>This method may be overridden to react to cancellation of the event,
     * however a super call must always be made as follows:
     * {@code CancelableEvent.super.setCanceled(canceled);}
     */
    @MustBeInvokedByOverriders
    default void setCanceled(boolean canceled) {
        ((Event) this).isCanceled = canceled;
    }

    /**
     * Returns the canceled state of this event.
     */
    @ApiStatus.NonExtendable
    default boolean isCanceled() {
        return ((Event) this).isCanceled;
    }
}
#

which does not need ASM and hides both methods 😛

spice sphinx
#

wtf is MustBeInvokedByOverriders

rigid gust
#

jb annotations

image.png
spice sphinx
#

oh it means must call super

rigid gust
#

yes

#

and so you can override setCanceled if you really want to, but you get a reminder to call super

inland imp
#

Yet for events with custom logic, that seems like a bad idea

spice sphinx
#

Still of the opinion that setCancelled must be overridden

rigid gust
spice sphinx
#

why as well ^

rigid gust
#

do you have an example of an event that cannot use that system?

inland imp
#

Well if I have custom cancellation logic

#

That setter is useless

#

So I override it to make it noop, now I get annoying warnings

#

That I must override it / call the super

spice sphinx
#

What could you possibly mean by that thonk

#

The most I've ever seen an event do on cancellation is set a result when the cancel flag is set

inland imp
#

Okey

#

Then I am fine

rigid gust
#

(if you really need to you can always @SuppressWarnings("MethodDoesntCallSuperMethod"))

spice sphinx
#

(also I think the interface could just be called Cancellable)

#

But that might have weird name conflict resolution with the annotation

rigid gust
#

the annotation is gone

#

also, single l

agile slate
rigid gust
#

isn't the standard in US english to use a single l?

agile slate
#

I'm not from the US

charred sluice
#

cpw's English, hence why there were two Ls before

spice sphinx
#

No they're both legal

rigid gust
#

there is a single L atm

#

and we will follow whatever it is that en-us does

agile slate
rigid gust
#

according to Merriam-Webster, the correct forms are canceled, cancellable and cancellation

#

so we should name the interface Cancellable but the methods isCanceled and setCanceled thonk

spice sphinx
#

screm

#

i hate English

charred sluice
#

That's American

rigid gust
#

blame yourself 😛

#

yes, and US spelling is the standard in computer science

agile slate
charred sluice
#

Seems strange to change dialect of an existing project that was English to American but if we're making breaking changes regardless... Meh

spice sphinx
#

there are uses of single and double L

rigid gust
#

the ABI uses single L only atm

charred sluice
#

Almost everything is inconsistent throughout our ecosystem

spice sphinx
#

isn't the param name double L?

rigid gust
#

param name is not ABI

#

we should standardize it to US spelling everywhere

gritty urchin
rigid gust
#

lol

#

anyway PR will be up in a few hours

wheat sinew
#

mismatched initialisms/acronyms are usually the work of the French

#

see also nearly every initialism involved with designing the modern metric system:
SI for International System of Units (Système International)
CGPM for General Conference on Weights and Measures (Conférence générale des poids et mesures)
CIPM for International Committee for Weights and Measures (Comité international des poids et mesures)
BIPM for International Bureau of Weights and Measures (Bureau international des poids et mesures)
and so on

#

as maty would say, vive la France!

granite nimbus
#

Also American here, canceled is by no means standard in the US. Plenty of Americans, myself included, use cancelled

#

So if you really want to go with American English don't let that rule out cancelled

#

Canceled is technically "more common" in American English but it's not almost all one or the other like it is with color/colour or the like

rigid gust
#

oh, do we keep the I prefix for interfaces?

#

for the interface name we have a few options: CancellableEvent, Cancellable, ICancellableEvent, ICancellable

rigid gust
pale grove
#

I think for consistency we should stick with I for interfaces and bring it up with the whole maintainers team later on

#

Because I have my reservations about it

#

But having this one interface not like all the others is silly

agile slate
#

well it is in the bus lib not neo

rigid gust
#

the other thing is that the event bus already contains BusBuilder which is an interface

#

admittedly BusBuilder is not modder-facing

rigid gust
#

went for ICancellableEvent

pale grove
#

We'll go for a sweeping refactor a later time if the maintainers agree. Though I don't expect many to, given that it's pretty much just a breaking change for breaking change sake, with no real benefit to anyone :p

outer steeple
#

there was a vote early on about the I prefixes in neo

#

it was unfortunately decided that we're keeping them

rigid gust
#

I suppose keeping them for old stuff and not using them for new stuff could work

pale grove
#

Isn't consistency something we should strive for in general?

#

Having a slowly diverging set of interfaces with disjoint naming schemes is pretty silly

#

Makes it a lot harder to search for stuff :p

rigid gust
#

we are inconsistent with the rest of the java ecosystem unfortunately

pale grove
#

We can at least be self consistent, and not intentionally make the entire framework way less intuitive to use :p

rigid gust
#

idk, I'm looking at the available options

#

I don't think the I prefix is fundamental when searching for something

pale grove
#

We either make everyone suffer in the short term by changing everything now, or we make slightly less people suffer for the rest of time by slowly changing things, or we keep consistency and intuitiveness by keeping the current system :p

#

I don't see many "options" here, everything is bad but one option is far less bad than the others

outer steeple
#

we already decided to make everyone suffer now

pale grove
#

By removing all the I prefixes?

#

Forcing everyone who uses an interface to fix the code :p

outer steeple
#

by changing packages, making breaking changes and reworking systems

#

everyone will suffer now one way or another

#

even if that implies changing a class/package name (we already need to provide migration tools for the package change anyways)

pale grove
#

hmm.

#

good point. I forgot about that

rigid gust
#

package change doesn't require migration tools?

outer steeple
#

should still provide a tsrg file for shrimp's tool

#

or a xml file for intellij migrations

#

and we can change the name of the class, not just the package in both of those

hushed ridge
#

if you do remove the I prefix; the class

class CancellableEvent extends net.blah.blah.CancellableEvent

is terrible imo. Rename one of them, and if I prefix is what it needs, then that's fine. But class + interface identically named (sans package) seems like a recipe for confusion, incorrect imports, annoyance, etc.

rigid gust
#

that is part of the tests so /shrug

#

I added the I prefix back yesterday, btw

rigid gust
honest timber
#

Event should be an abstract class

pale grove
#

the point is to move away from magic annotations :p

granite nimbus
#

Yeah I really dislike the use of a magic annotation for this. I feel like making unlistenable stuff have to be abstract is fine. After all, if it's unlistenable you shouldn't be able to instantiate it...

#

The reasoning against that was that then every abstract event would be unlistenable, right?

#

If you really want to use a magic annotation, it's much cleaner to have a ListenableAbstractEvent one. That way most of the time you never have to use it - and if you design your API to avoid it you actually never have to use it

#

Basically, is there any use case for having a non-abstract event be unlistenable? No. All concrete types should be listenable - because they can be instantiated, and thus fired on the bus

rigid gust
#

The problem is that we might have listenable abstract events

granite nimbus
#

Yes #1136448404495532133 message

rigid gust
#

@UnlistenableEvent requires abstract

#

I prefer my use of annotations

#

Overloading the meaning of abstract is also a form of magic

granite nimbus
#

This would all be fixed with a proper event type system... maybe I'll put together a proof of concept of what that would look like.

rigid gust
#

That's probably not compatible with mod busses

rigid gust
#

@outer steeple can you look at the ICancellableEvent PR when you have the time?

#

UnlistenableEvent too ideally... for that we mostly need to decide whether we want the feature

outer steeple
#

wait

#

screm

#

isn't the names class useless considering the removal of asm

rigid gust
#

Yeah I have to remove it

#

I'll remove it with the rest of the ASM stuff once I've done more tests with LMF

granite nimbus
#

I'll try to prototype it once I have less other projects

rigid gust
#

There should be a single mod bus type but many mod busses

#

There's probably a way to make it work but I doubt it would be much better than the current eventbus

rigid gust
granite nimbus
outer steeple
rigid gust
#

I know Quilt has sort of replacement but I prefer Event subclasses to Fabric-style functional interfaces because they are better for forward-compatibility

latent rivet
#

Btw, Tech, it's worth noting that IEventBus#stop() currently documents that event posting after bus shutdown is no-op, so that would need to be adapted if we do throw on a stopped/not-yet-started bus

rigid gust
#

I see

#

I really don't know about that tbh, I don't like the idea of these start and stop methods in the first place heh

granite nimbus
rigid gust
#

sure

#

as usual: no promises 😉

granite nimbus
#

You mean, besides that folks will give it fair consideration and weigh it on its merits, and not just go with a status quo bias? 😉

agile slate
granite nimbus
#

I don't see why there must be

#

After all, there wasn't really with the capability overhaul... and I'm not suggesting changes to how people listen to events. Or. Well. Minor changes there. But most of what I'm thinking of would affect new events added, and how you create events

agile slate
#

we are in a major breaking changes phase where something like eventbus (now bus) can break and capabilities is also in this window ... yours would be outside that window

granite nimbus
#

I mean I've been suggesting this sort of approach for ages but kept being told "no we're not doing that"; in fact, tech even said a goal of his EventBus PRs was not to make major changes...

#

So... I don't really know what to say here? Like, if the approach you've had from the very beginning was "don't make big changes to EventBus" you can hardly expect me to have put together my big changes by now

#

And breaking change windows will pop up again. It's not like I'm advocating for such a large overhaul as, like, switching to Fabric's system or something

#

If I implement it all the way I'm thinking it wouldn't require changes to anyone using SubscribeEvent or EBS. Heck, I could probably make all the current API for listening to events keep working

#

I'm just tired of being told "we don't want big changes" and, basically, being told that you're likely not going to consider my proposal on those grounds alone. That's basically you all saying "don't bother if it's a big change, since we'd never merge it anyways". No, you don't get to say that. You wait till it exists, you judge it on its merits, not on the status quo, you see how big of a change it would be, and if it's better than the current system and thus something you'd want you find an appropriate timeline or intermediate system to transition stuff. If it isnt better, you provide the reasoning that means it doesn't work for neo, and that can't just be "changes are too big"

#

And sure, I might make such a system and you guys might decide it's not what neo needs. But not on the grounds of "if it ain't broke don't fix it"

#

That's a reason to delay stuff till a better time but not much more, especially when it's someone else putting in the time to do said fixing

rigid gust
#

the only thing I said is that there is no guarantee that your prototype will be used

#

but you should feel free to make one

#

the differences between that and the current system will be weighted against the benefits it brings

granite nimbus
#

My giant wall of text there was targeted at schurli's justification of status quo bias as a legitimate factor when weighing the advantages of something someone else has already made

#

The "no promises it will actually be used" bit I would assume as a given. I would also assume the stuff I mentioned here: #1136448404495532133 message, which was why I mentioned it

#

screm why did that embed

rigid gust
#

difference with the current status quo is a decision factor

#

I'm not planning to use that as an excuse to shut down alternative designs

granite nimbus
#

You compare it against the status quo, yes. But when someone else has already done the work of making the version, the status quo version doesn't get any points just for being the existing solution

rigid gust
#

but if we have multiple "equivalent" designs then we should go for the one that will break the least things

#

the change has to be done by everyone in the ecosystem, not just by Neo itself... so "the work is already done" is not necessarily an argument

granite nimbus
rigid gust
#

anyway, it was just a small disclaimer, please have a look and see if you can do something better

granite nimbus
#

Or you build tools to help people transition, or whatever

rigid gust
#

I don't want to argue this, my position can be summarized as "no change just for change's sake"

granite nimbus
#

Or you find a way to shoehorn the old API to fit it - which is my plan

granite nimbus
#

And I'm tired of seeing "no change for change's sake" used as a way to try to shut that down. The simple fact that it's change doesn't have a say in whether it's a good idea or not - if the system were built from scratch today, and you'd pick A over B in that scenario, then there should be a plan to move from B to A

#

The part that actually matters is whether A is actually better than B

rigid gust
#

the way old designs inform a choice for me is "does the benefit of B compared to A outweigh the ecosystem migration cost from A to B?"

granite nimbus
#

Yeah no. That's relevant in terms of when you make the switch. It determines timeline. It determines figuring out easy ways to transition. It shouldn't determine, or be relevant to, whether the switch is planned to eventually happen

agile slate
granite nimbus
#

And I'm saying that's just not how stuff should work, basically. That isn't a relevant factor in terms of whether to make a change. It's relevant in the "how you make the change" bit, but not in whether you should

rigid gust
humble junco
#

status quo bias is fine

#

status quo is where people have the expertise, it requires the least immediate effort, so it is necessary to weight the long term improvement vs the short them pain

rancid pasture
#

is there any plan to change the way event subclasses work? for example some have .Pre and .Post which are errorprone since one might accidentally register the superclass making the event firing twice. Other events use internal Phases instead like player tick event which if you dont check for it it will fire 4 times, for client, server and phase start and beginning. I've noticed that for a registered event that checks for phase immediately, the other phase still takes like 0.04% of server time which is small but could be better

rigid gust
#

client/server and pre/post as fields is not something that class type checks can guard against

rancid pasture
#

oh thats nice

rigid gust
#

thinking more about this, we should probably use abstract as a marker for unlistenable events

#

I think that in 99% of the cases it will be equivalent to the annotation, so might as well use it for that

primal mist
#

yeah, especially given

  • it's honestly not very reasonable for event classes (even unlistenable ones) to have any actual unimplemented methods anyway, other than the event machinery itself they're pretty straightforward data containers with simple getters/setters
  • there is, therefore, no good reason for a listenable event to be abstract
rigid gust
wheat sinew
#

would there ever be a reason for an event to be cancellable but a subclass event not be cancellable?

haughty current
#

Apologies for silly typo nitpick - thank for you for putting that together, was very helpful! :)

-  We will address this in capability rework.
+  We will address this in the capability rework.
wheat sinew
#

should I also bring up the comma splice I noticed

rigid gust
#

hmmm?

rigid gust
rigid gust
# 
Benchmark                                                 Mode  Cnt    Score    Error  Units
EventBusStressTest.testClassLoader                        avgt   15  124.391 ▒ 15.395  ns/op
EventBusStressTest.testClassLoaderCancellable             avgt   15  183.628 ▒ 30.735  ns/op
EventBusStressTest.testLambdaMetaFactory                  avgt   15  100.639 ▒ 19.911  ns/op
EventBusStressTest.testLambdaMetaFactoryCancellable       avgt   15  173.024 ▒ 42.990  ns/op
EventBusStressTest.testMethodHandles                      avgt   15  511.020 ▒ 70.839  ns/op
EventBusStressTest.testMethodHandlesCancellable           avgt   15  583.273 ▒ 57.246  ns/op
#

I think we have a winner here

agile slate
#

LMF ftw

rigid gust
#

yeah!

honest timber
#

how does the idea of events looks like?

rigid gust
#

Inheritance of @SubscribeEvent methods might have to be deleted

wheat sinew
#

that's the first I've heard of that annotation thinkies

rigid gust
#

Fixed lol

wheat sinew
rigid gust
#

Atm you can register an object whose superclasses and superinterfaces have methods with @vernal latch

granite nimbus
#

I don't think killing that is necessarily a bad thing

rigid gust
#

The problem is that this doesn't interact nicely with private/package-private/protected modifiers

#

For public we can do getMethods, but this doesn't work to get the full list of protected methods, for example

#

And I just removed the ASM transformer that changes every non-private @vernal latch method to public

#

As far as I can tell, method overriding isn't done properly atm - the actual method that is used is picked randomly among the candidates

#

It doesn't even detect methods in interfaces lol, the code is so broken

#

I'm fine with a range of solutions

#

The simplest would be to remove this sort of inheritance

#

We could also pull all methods but disallow overrides

mellow hearth
#

hmmm do we know if anyone is actually using this behaviour?

#

Im in favour of yeeting it unless theres an actual valid usecase someone needs it for

primal mist
#

can we detect it and warn/error?

#

i don't do object event registration at all so idk the use cases

agile slate
mellow hearth
#

Maybe? That would be a question for @outer steeple
I think we're already saving annotations and inheritance so we might already be good?

rigid gust
#

Maybe

#

I'll remove it and error-proof the whole thing

#

(will also throw errors when the eventbus expects a static method but the method is not static, etc...)

latent rivet
rigid gust
#

I'm checking the superclasses for SubscribeEvent annotations and throwing an error if this happens now

#

Unfortunately my battery ran out so this isn't pushed yet 😛

granite nimbus
hushed ridge
granite nimbus
#

Hence the issue

granite nimbus
agile slate
rigid gust
agile slate
#

ik, I didn't say it is a good idea just that it works

granite nimbus
#

(Wait I realized making subscribe methods final doesn't solve this; yeah, tech, your solution makes sense to me)

rigid gust
#

I hope it will also reduce the amount of confusion caused by SubscribeEvent with novice modders (forgot to make a method static etc)

pallid kraken
#

Can the annotation be removed completely?

rigid gust
#

Ah yeah we can do that

mellow hearth
#

Can we?

#

Unless we remove the object registration I don't think we can

latent rivet
#

You'd have to remove every listener registration apart from addListener()

pallid kraken
#

addListener(Consumer<Event>) may work

mellow hearth
#

We already have addListener

rigid gust
#

?

#

Just register all methods as event handlers

#

Maybe not the best idea

pallid kraken
#

You are right though, each listener method would need to be registered separately. Personally I think that is fine; it works like this on Fabric and it would significantly simplify the code. Also, less magic annotations.

mellow hearth
# rigid gust ?

I assume their talking about adding addListener, we already have it

#

And I agree, that's not the best idea to register all methods

pallid kraken
#

I know it already exists; I just forgot what its signature is

latent rivet
#

Automatically registering every method in a class is significantly worse than a magic annotation in my opinion. Requiring individual listener registration (i.e. removing EBS as well) would prevent hands-off registration of client-only listeners and listeners in other source sets and while the former can be replaced with individual registration behind a side-guard, the latter cannot

granite nimbus
#

If you're allowing register(someInstanceWithMethods) or register(SomeClass.class) at all, you need to keep the annotation

#

If you're rewriting event bus to the degree that that sort of registration is getting entirely yeeted, then I would want to push for a bunch of other changes at the same time

agile slate
#

It can't ... listeners in other source sets is a major problem

granite nimbus
#

While I agree that with that sort of stuff in mind that seems like a good argument for keeping object/class based listener registration around... I would also argue that that's a good argument for allowing multiple mod entrypoints, because even funky source set layouts should never force a mod to use EBS for stuff

#

Basically: if you want weird magic side effects, please make them use an annotation at least

#

But we should provide approachs that don't need weird magic stuff

rigid gust
#

It's not magic, we can clearly document what it does

granite nimbus
#

Consider the following. A user uses the instance of their main @Mod class to hold listeners. This is not an uncommon thing to do. They register that instance, which has some subscribed methods on it, with the forge bus. They've got a couple of extra listeners that need the mod bus though, so they shove those in unannotated methods and register the methods directly to the mod bus. I see no reason to make this into a flawed approach

rigid gust
#

It's super confusing but indeed we won't prevent that

granite nimbus
#

All of that said, the use of the event also makes stuff explicit. A class could have any number of methods that take a Something as an argument. We're either 1) special casing ones that consume events, and any sort of non-obvious special casing is not good (that's like the definition of "magic" in this context), or 2) not allowing classes to have other non-listener methods that might look sorta like listener methods, which is also not good

rigid gust
#

Well idk, instance methods could indeed be useful

granite nimbus
#

Yeah, exactly - that's why @SubscribeEvent should still be needed

rigid gust
#

sorry for the massive PR (most of it is ripping out ASM and MML), have fun

#

almost done with the eventbus changes heh - I will need to make FML changes too for better phase support at some point

wheat sinew
#

but what if I want to have a class which has both static listeners and instance listeners thinkies

gusty dove
#

no more event transformer

rigid gust
#

well don't 😛

#

I think the ability to detect user mistakes is worth sacrificing whatever ugly patterns "advanced users" have come up with

#

the new system should be a lot more beginner-friendly

#

(I hope)

agile slate
#

is it actually useful to have non static listeners cause storing stuff between event calls is cursed

pale grove
#

As long as the new ways are consistent so we don't need !events I can go with whatever

pale grove
agile slate
pale grove
#

My event test framework relies on being able to store data between events

#

To ensure that the data is changed/set/read properly between them

#

Yeah, but it makes verifying the entire API a lot harder

#

Not against it, just unsure how it'd work if we had that limitation

granite nimbus
#

Instance listeners are super useful

#

Especially if you don't store your blocks and whatnot statically but in your mod object

rigid gust
granite nimbus
#

But this looks great to me

#

If someone is using static and instance SubscribeEvent on the same class there's bigger issues

agile slate
granite nimbus
# agile slate that is cursed

Nah, it makes multiloader dev a lot easier in some ways. Turns out the 50% of the pain points in multiloader stuff are solved if you treat the whole mod as operating on a mod instance

#

And having side effects in static init is never good in my mind anyways

agile slate
#

I think you are the only person doing this

rigid gust
haughty current
#

What do we actually need the IMPL_LOOKUP for? Is that just to call protected methods?

rigid gust
#

To call LambdaMetaFactory

#

I need the module flag and there's no other way

haughty current
#

Ahh, because LambdaMetafactory puts the generated class under the method's package, rather than the Lookup's package?

rigid gust
#

Yes exactly

#

Well no

#

The lookup and method must have the same package

#

The lookup needs private access so it is always of the method's class, but we also need the module flag to define classes

haughty current
#

So MethodHandles.lookup() returns something with both those flags set.

#

I'm sure you're right! I've just forgotten what LambdaMetafactory does under the hood that means one needs it.

#

If only MethodHandleProxies.asInterfaceInstance was actually fast, would make this much easier to do :).

rigid gust
#

I spent some time stepping through LMF to understand why it needs full access - that's because it basically generates class files with ASM

#

IMO there should be a supported way to get package access for open modules, maybe something to bring up with the jdk devs one day 😉

granite nimbus
#

I still have to look at how method handle lookups work with modules. That's the bit I never quite understood, and I should probably make OpenSesame support it

haughty current
rigid gust
#

I can't get the MODULE bit on a lookup for a class in another module sadly

haughty current
#

Okay, I've made it work, but only for public methods.

#

Personally I'm not a fan of protected/private listener methods, but not clear to me what the tradeoffs are with breaking them.

#

To explain, there's a couple of fun method handle things:

  • Doing LOOKUP.in(callbackClass) clears the module/private bits for a non-trusted Lookup instance. So we just remove that, and use MethodHandle.lookup() directly.
  • This then allows us to declare classes, but causes access checks to fail. To fix that, we need to .addReads(callbackClass.getModule()).
  • Theoretically we should be able to do callback.setAccessible(true) too and call that, but LMF uses MethodHandles.revealDirect under the hood, which does another access check.
#

Ironically if one just reimplemented LMF, it'd work fine :/.

haughty current
#

Happy to put together a PR if that would be helpful, just to demonstrate? Sorry, not trying to undermine your work, just the number of --add-opens Neo uses already makes me sad :D:.

outer steeple
#

it avoids other mods calling your event handler or you yourself doing it

#

my @SubscribeEvent listeners are basically always package-private

granite nimbus
#

If I understand it right this would let them be private-private, right? That would be awesome

#

Easy way to prevent mistakes

#

(and clean up your autocomplete)

haughty current
#

Obviously this doesn't remove the use of asm (though does remove the mod launcher/asm code), so may not be worth it.

outer steeple
latent rivet
#

There's a rogue > at the end of the link

outer steeple
#

you have a leading > at the -

haughty current
#

Thank you, clearly missed the leading <.

outer steeple
haughty current
#

Yeah :/. The way it was written made sense when Java 8 came out, but now that we've got fancier methodhandle stuff, it's odd that its retained its limitations.

outer steeple
#

I wrote that while needing to generate interfaces using LMF but non-direct handles

#

with somethind like that we could easily build the IEventListener out of handle chains and lmf

haughty current
#

Wait, I'm so confused what that code is trying to do. How do you end up consuming that generated invoke method?

outer steeple
outer steeple
#

it's basically a direct wrapper around another handle

#

tho due to the fact that the defined class is hidden i've ran into some classloader pain

haughty current
#

Can you not just invoke the original MH directly though? I'd expect the underlying LF to get specialised and the performance to be the same.

#

Though realise "I expect" without benchmarking is worth nothing :p.

outer steeple
haughty current
#

Ooooh, that's what you're trying to do.

#

Gotcha then.

#

It feels like it would have been less code just to reimplement LMF rather than implement that :p.

outer steeple
#

MethodHandles.dropArguments(lookup.findVirtual(), 0, SomeParam.class) can't be used using lmf nor can

    public static <T> MethodHandle createArrayIterationHandle(Class<T> type, Method target) throws Throwable {
        final var arrayType = type.arrayType();
        final var targetHandle = MethodHandles.publicLookup().unreflect(target);

        // Get a method handle for the array's getter
        MethodHandle getter = MethodHandles.arrayElementGetter(arrayType);

        // The array getter but with reversed parameter order
        MethodHandle reorderedGetter = MethodHandles.permuteArguments(getter, MethodType.methodType(type, int.class, arrayType), 1, 0);

        // The target but with the loop position and array inserted after the instance parameter so that we can fold the array getter into the target
        MethodHandle extraTarget = MethodHandles.dropArguments(targetHandle, 1, int.class, arrayType);
        // The body of the loop. Calls the targetHandle on the array element at the position
        MethodHandle body = MethodHandles.foldArguments(extraTarget, 0, reorderedGetter);

        final Class<?>[] additionalArguments = target.getParameterTypes();
        return MethodHandles.countedLoop(
                MethodHandles.dropArguments(MethodHandles.constant(int.class, 0), 0, Lists.asList(arrayType, additionalArguments).toArray(Class[]::new)),
                MethodHandles.dropArguments(MethodHandles.arrayLength(arrayType), 1, additionalArguments),
                null,
                body
        );
    }
#

it's an extremely annoying and arguably somewhat arbitrary limitation

#

considering lmf already has cases where it uses a classdata-injected handle and invokeexact's it instead of a INVOKEVIRTUAL/STATIC

haughty current
#

Oh yeah, they could remove that limitation just by deleting code. It's definitely odd.

outer steeple
#

inb4 their reason for not doing it is backwards compat hahayes

#

"we can't remove it because people that expect it to crash will not crash"

haughty current
#

I guess the Javadoc does say it only accepts direct handles. The (arguably cleaner) alternative would be to add a version of MethodHandleProxies.asInterfaceInstance which generates classes per-MH rather than per-interface.

spice sphinx
#

imo requiring final is fine

#

it can delegate to a virtual

outer steeple
#

til methodhandleproxies exists

#

oh eww it is actually a proxy

#

why oracle

haughty current
outer steeple
#

proxies are such a mistake

charred sluice
#

At this point maybe require Java 22 for NeoForge? 😛

outer steeple
#

oh god

#

why did I make the mistake of looking at ProxyGenerator

#

it really shows how old the code is

rigid gust
#

I think the --add-opens is not a big deal heh

#

I thought that defineInnerClass needed package access, how did you get around that?

#

The generated class needs to be in the same module as the listener object IMO

haughty current
rigid gust
#

Aren't there access restrictions if it's not in the right package? I should look at what exactly you're doing heh

haughty current
#

Oh, it's not in the right package (it lives in the bus package), but I guess my feeling is that doesn't matter.

rigid gust
#

Well if it works it works 😛

rigid gust
#

Oh I just understood what you're doing heh

#

You're accessing the target via a static final MethodHandle which can be inlined by the jit

#

Really unfortunate that MethodHandleProxies is too slow atm, it looks like it does exactly what we need

haughty current
rigid gust
#

Oh I didn't notice that LMF was doing that

#

I think my preference goes to the add-opens arg over your solution, just because I already wrote it, tested it, and benchmarked it. But yours is perfectly valid! 😉

haughty current
#

Oh yeah, that's completely fair. I'm irked by the poking of jdk internals, but me being irked is not a good reason to throw things away :p.

agile slate
#

you could do benchmarks yourself and check if it'd work

haughty current
#

I would eat my hat if the numbers are different between the two, but happy to if that would be useful.

rigid gust
#

It's more that I don't have too much time this weekend unfortunately heh

haughty current
#

Yeah, I hadn't realised quite how long the benchmark suite took to ran :p.

rigid gust
#

I also like the idea of getting rid of asm

#

I usually change the 5s to 1s or 2s

haughty current
#

I do in principle! But I think if you replace ASM (nasty) with poking JDK internals (also nasty!) I don't think it's much of a gain :p.

#

I think for me the main bit is getting rid of the mod launcher/access transformer code, which both approaches do :).

rigid gust
#

Yeah that's a nice benefit

#

Either way we can change to MethodHandleProxies in a few years ^^

#

I suppose ASM is less ugly

#

(at least it's a supported API)

haughty current
#

We could generate the classes at compile time and remove the runtime ASM dependency (/s, I think).

rigid gust
#

Please no 😛

agile slate
#

hmmm an AP for @vernal latch and @Mod.EventBusSubscriber thinkies

agile slate
light mantle
#

I think this is Finn's secondary acc harold

rigid gust
#

Just kick it

light mantle
#

and it exists purely to spite us

haughty current
# haughty current I would eat my hat if the numbers are different between the two, but happy to if...

Not especially convinced by how stable the performance is on my machine for benchmarking, but:
My branch (c418eb2b1805e5b71157eed04a26420297a7b5fc)

EventBusBenchmarkNoLoader.testNoLoaderCombined         avgt   15  76.987 ± 0.831  ns/op
EventBusBenchmarkNoLoader.testNoLoaderDynamic          avgt   15  34.868 ± 0.982  ns/op
EventBusBenchmarkNoLoader.testNoLoaderLambda           avgt   15  27.928 ± 0.363  ns/op
EventBusBenchmarkNoLoader.testNoLoaderStatic           avgt   15  31.809 ± 0.588  ns/op

Main (86e69cea88d0acb1b108b0ad4f7aa8bc5d08e4f6)

EventBusBenchmarkNoLoader.testNoLoaderCombined         avgt   15  78.481 ± 1.641  ns/op
EventBusBenchmarkNoLoader.testNoLoaderDynamic          avgt   15  33.836 ± 0.404  ns/op
EventBusBenchmarkNoLoader.testNoLoaderLambda           avgt   15  29.738 ± 1.837  ns/op
EventBusBenchmarkNoLoader.testNoLoaderStatic           avgt   15  33.147 ± 1.171  ns/op
agile slate
#

soo can we have a direct comparison between just your impl and techs LMF impl

haughty current
#

Yeah sure! Give me ... uh, 7 minutes.

rigid gust
#

That benchmark is suboptimal for performance testing

#

You should copy the "many listeners" benchmark from my PR

haughty current
#

I guess the main difference is that ASM needs an explicit cast for the instance, while LMF doesn't, and that's not really avoidable. Apologies for the diversion here then!

haughty current
#

I'm a little concerned by that benchmark though - because there's only one listener, everything is going to get super specialised. Doing a quick test with interleaving three listener classes shows a 6x slowdown in those benchmarks.

rigid gust
#

Why would LMF not need a cast?

rigid gust
haughty current
# rigid gust Why would LMF not need a cast?

As in a cast on the instance object - the LMF class has a field of the receiver type (and so doesn't need a class), but the ASM just uses j.l.Object and casts within the method handle.

rigid gust
#

ASM could use the exact object class heh

haughty current
#

Not if that class is package private, alas. You have to avoid all references to external types :/.

haughty current
rigid gust
#

Can't you adapt the methodhandle to accept objects?

#

I thought that would elide the cast with the jit

haughty current
#

Oh, so the methodhandle does accept objects, but it then needs to do a checked cast within the MH itself.

#

^ To be clear, I haven't dug out jitwatch to confirm this.

#

I've been doing some more fiddling, and I'm not sure how much you want to change/break the API here, but if it was okay to change IEventListener to be an abstract class, you can get some pretty reasonable gains in the heterogeneous listeners case (source code, if it's useful).

agile slate
#

not an option we use it as lambdas

outer steeple
#

the fact that event listeners can be lambdas doesn't mean the wrapper can't be an abstract class

haughty current
#

^ Oh yeah, to clarify .addListener accepts a Consumer rather than an IEventListener. From what I can tell, we don't construct IEventListener outside of Bus, but I may have missed something.

outer steeple
#

that looks reasonable to me

agile slate
#

yea then try the abstract class thing

rigid gust
#

I don't think there's even IEventListener lambdas in eventbus atm

#

Sounds fine to me

#

If you'd PR to my PR that would be best cause there's also the other changes I made that I'd like to keep

charred sluice
outer steeple
#

PR to the PR PR'ing the PR

#

a lot of time is probably spent just casting the events

#

i wonder if there's a clean way to reduce the number of casts from the number of listeners to 1

rigid gust
#

The cast is probably optimized by the JIT