Skip to content

Use hidden classes for event executors#11690

Closed
SirYwell wants to merge 1 commit into
PaperMC:masterfrom
SirYwell:simplify-event-executor
Closed

Use hidden classes for event executors#11690
SirYwell wants to merge 1 commit into
PaperMC:masterfrom
SirYwell:simplify-event-executor

Conversation

@SirYwell
Copy link
Copy Markdown
Contributor

This change replaces the previous approach using ASM with a simpler one only depending on MethodHandles and hidden classes.

The concept is pretty straightforward: We have a template class that contains all the logic. This class is never loaded directly, but its bytecode is used to define hidden classes. Each hidden class gets its own MethodHandle (and the other attributes needed). This way, the attributes can be static, allowing full optimization by the JVM. This is the main difference compared to the previous MethodHandle-based implementations.

The generated classes previously had the additional problem of book-keeping: The classes needed to be loaded by a class loader, and - as plugins can be unloaded - the classes also shouldn't outlive their plugin.
Hidden classes can be unloaded even if their defining class loader is still reachable, so the JVM takes care of that this way.

One difference is that previously one class was loaded per method, even if a method was registered multiple times. Now, each time a method is (re-)registered, a new class will be loaded. I assume that this isn't a problem in real world use-cases and argue that the simplification justifies that.

I manually verified that all combinations of private/public, static/non-static, void/non-void still work correctly.

@SirYwell SirYwell requested a review from a team as a code owner November 30, 2024 12:40
@Owen1212055
Copy link
Copy Markdown
Member

Is it possible at all to get some rudimentary performance benchmarks here? I assume this new strategy is better, but would love to see. 🙂

@SirYwell
Copy link
Copy Markdown
Contributor Author

SirYwell commented Dec 4, 2024

Is it possible at all to get some rudimentary performance benchmarks here? I assume this new strategy is better, but would love to see. 🙂

Measuring what's going on in real applications is rather difficult. I micro-benchmarked the changes though, and there this new approach performs better (I'm actually surprised by how much, not sure if there's something wrong with the benchmark). See https://gist.github.com/SirYwell/c8ad6bd2a1cac1f6324f3a65219dffcd (note that the , in the results is a decimal point from my understanding)

@electronicboy
Copy link
Copy Markdown
Member

One difference is that previously one class was loaded per method, even if a method was registered multiple times. Now, each time a method is (re-)registered, a new class will be loaded. I assume that this isn't a problem in real world use-cases and argue that the simplification justifies that.

This was done because there are plugins that will continually un/register handlers which caused issues as those generated classes would pile up and run the JVM out of classspace

@emilyy-dev
Copy link
Copy Markdown
Member

emilyy-dev commented Dec 4, 2024

One difference is that previously one class was loaded per method, even if a method was registered multiple times. Now, each time a method is (re-)registered, a new class will be loaded. I assume that this isn't a problem in real world use-cases and argue that the simplification justifies that.

This was done because there are plugins that will continually un/register handlers which caused issues as those generated classes would pile up and run the JVM out of classspace

If I'm not mistaken this is where the weak relationship that hidden classes have with their classloader would be helpful, the class can be unloaded and forgotten after all refs to it are no longer reachable, unlike normal classes

@masmc05
Copy link
Copy Markdown
Contributor

masmc05 commented Dec 4, 2024

https://openjdk.org/jeps/371

A hidden class is not created by a class loader and has only a loose connection to the class loader deemed to be its defining loader. We can turn these facts to our advantage by allowing a hidden class to be unloaded even if its notional defining loader cannot be reclaimed by the garbage collector.

So most probably unregistering handlers would automatically allow jvm to deal with unloading the hidden class

@SirYwell
Copy link
Copy Markdown
Contributor Author

SirYwell commented Dec 4, 2024

Yes, that would only be a problem with -XX:-ClassUnloading, but I don't think this is a scenario we should have extra complexity for. When it comes to performance, unregistering and re-registering handlers frequently is bad anyway.

@Warriorrrr
Copy link
Copy Markdown
Member

Superseded by the mentioned PR above

@Warriorrrr Warriorrrr closed this Dec 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Closed

Development

Successfully merging this pull request may close these issues.

7 participants