Skip to content

Avoid resolving a MethodHandle for every event listener invocation - #747

Open
BonsUnleashed wants to merge 1 commit into
architectury:1.20from
BonsUnleashed:bons-furious/architectury
Open

BonsUnleashed wants to merge 1 commit into
architectury:1.20from
BonsUnleashed:bons-furious/architectury

Conversation

@BonsUnleashed

@BonsUnleashed BonsUnleashed commented Sep 29, 2026 •

Copy link
Copy Markdown

EventFactory.invokeMethod runs MethodHandles.lookup().unreflect(method).bindTo(listener).invokeWithArguments(args) for every listener call, so each call looks up, binds and adapts a new handle. This keeps one handle per Method in a ConcurrentHashMap: unreflected once, adapted with asSpreader and asType to (Object, Object[])Object, and called with invokeExact. Arguments, return values and listener exceptions reach callers as before, and the stack trace between a listener and invokeMethod has no reflection frames.

In a JDK 17 microbenchmark with one listener and two arguments, a call went from about 860 ns and 1.5 KB to about 18 ns with no allocation (JDK 21: about 800 ns to 18 ns).

Compiles on 1.20 (common and forge). Not tested in game on this branch.

Bons and Furious currently ships a patch for this: CurseForge · Modrinth · GitHub

@CLAassistant

CLAassistant commented Sep 29, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@shedaniel

Copy link
Copy Markdown
Member

Would storing the method handle produce the same level of performance gains? i.e. I think the original idea to unreflect was to reduce stacktrace reflection traces

@BonsUnleashed

Copy link
Copy Markdown
Author

Storing the unreflected handle alone doesn't get there, because bindTo and invokeWithArguments are most of the cost. Caching a spreader handle per method and calling it with invokeExact does, and it keeps the short stack trace. JDK 17 microbenchmark, one listener with two arguments:

ns/call bytes/call frames between listener and invokeMethod
current (unreflect per call) 717 1504 2
this PR (Method.invoke) 3.4 0 4
cached handle, bindTo + invokeWithArguments 447 1168 2
cached spreader, invokeExact 12 0 1

On JDK 21 the cached spreader is about as fast as Method.invoke (14 ns against 18 ns in a similar run). I can switch the PR to the cached spreader if you prefer that.

@shedaniel

Copy link
Copy Markdown
Member

The cached spreader option sounds good, would appreciate if you can switch to that! Thanks!

@BonsUnleashed
BonsUnleashed force-pushed the bons-furious/architectury branch from 96cd7e4 to aab2410 Compare September 29, 2026 22:53
@BonsUnleashed

BonsUnleashed commented Sep 29, 2026 •

Copy link
Copy Markdown
Author

Thanks for the suggestion! Switched in aab2410: one MethodHandle per Method, kept in a ConcurrentHashMap, unreflected once and adapted with asSpreader + asType to (Object, Object[])Object, then invokeExact per call. Listener exceptions propagate unwrapped as before, and there are no visible frames between a listener and invokeMethod (the original showed invokeWithArguments; Method.invoke showed two or three accessor frames).

Same microbenchmark as above, 5M calls, best of 5:

JDK 17 JDK 21 bytes/call
original (unreflect per call) 860 ns 800 ns 1,504
Method.invoke (previous revision) 8 ns 18 ns 0
cached spreader behind the map 18 ns 18 ns 0

The map lookup costs about 4 ns over a static handle. A 30-case comparison against the original (return values with boxing, unboxing and void; thrown objects by identity; null receiver) passes on both JDKs. Description updated, compiles on common and forge. Thanks again, it's the cleaner version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants