Skip to content

Conversation

@chrisvest
Copy link
Member

@chrisvest chrisvest commented May 3, 2024

Motivation:
We've been using reflection as a way to abstract over Java API versions. Now that we're based on Java 8, some of this reflection usage is no longer necessary. In some cases, the APIs we were reflecting on are now directly available. In other cases, we'd like a little more performance and can call through method handles instead.

Modification:
Remove some reflection usage that was necessary when running on Java 6 or 7, and replace it with direct calls if the code is still needed. Replace the more performance sensitive reflection usage with MethodHandles.

Also remove the animal sniffer maven plug-in, because animal sniffer does not support signature polymorphic method calls.
See mojohaus/animal-sniffer#67

Result:
Cleaner, and perhaps slightly faster, code.

Fixes #14009

Motivation:
We've been using reflection as a way to abstract over Java API versions.
Now that we're based on Java 8, some of this reflection usage is no longer necessary.
In some cases, the APIs we were reflecting on are now directly available.
In other cases, we'd like a little more performance and can call through method handles instead.

Modification:
Remove some reflection usage that was necessary when running on Java 6 or 7, and replace it with direct calls if the code is still needed.
Replace the more performance sensitive reflection usage with MethodHandles.

Result:
Cleaner, and perhaps slightly faster, code.
Animal Sniffer inspects bytecode call-sites to look for calls to methods not defined by Java 8.
However, it doesn't know about signature polymorphic methods, like MethodHandle.invokeExact(), so we will always get warnings about those calls.

Since we can now natively build on Java 8 (this was previously not possible with Java 6), we can instead rely on the test suite to make sure we don't make calls to methods not available in Java 8.

The associated animal sniffer issue is mojohaus/animal-sniffer#67
@chrisvest chrisvest linked an issue May 3, 2024 that may be closed by this pull request
@chrisvest
Copy link
Member Author

Not all reflection usages have been replaced, because 1) reflect gives access to annotations, and method handles don't, and 2) some cases are just not performance sensitive so it's not worth the effort to change code that works.

Copy link
Contributor

@franz1981 franz1981 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to give another round but overall look fine!

@franz1981
Copy link
Contributor

franz1981 commented May 4, 2024

@chrisvest The allocate uninitialized arrays method, if uses Methodhandle::invokeExact doesn't need to have any limit: it will just go faster in any case.
The limit in bytes was because reflective calls over intrinsic don't get a good treatment (it's using some old code path, which was never fixed by https://openjdk.org/jeps/416 too!), but with MethodHandle this is not true!

Additionally, I think we can target the private method instead of the public one -> https://github.com/openjdk/jdk/blob/9347bb7df845ee465c378c6f511ef8a6caea18ea/src/java.base/share/classes/jdk/internal/misc/Unsafe.java#L1385

@chrisvest
Copy link
Member Author

@chrisvest The allocate uninitialized arrays method, if uses Methodhandle::invokeExact doesn't need to have any limit: it will just go faster in any case. The limit in bytes was because reflective calls over intrinsic don't get a good treatment (it's using some old code path, which was never fixed by https://openjdk.org/jeps/416 too!), but with MethodHandle this is not true!

Oh, you mean we can remove the UNINITIALIZED_ARRAY_ALLOCATION_THRESHOLD checks?

Additionally, I think we can target the private method instead of the public one -> https://github.com/openjdk/jdk/blob/9347bb7df845ee465c378c6f511ef8a6caea18ea/src/java.base/share/classes/jdk/internal/misc/Unsafe.java#L1385

The PlatformDependent method is public, so I think it's best if we keep the input validation checks.

@chrisvest chrisvest force-pushed the 4.2-method-handles branch from c82eca9 to 7816e6b Compare May 4, 2024 22:13
@chrisvest
Copy link
Member Author

Lot's of failures in EpollSocketHalfClosedTest.testHalfClosureOnlyOneEventWhenAutoRead, so that needs to be looked at.

chrisvest added 2 commits May 5, 2024 09:07
Method handles don't work so well on Graal, and lookup may throw InternalError instead of NoSuchMethodException.
This should make our reflection hacks work on Graal.
@chrisvest
Copy link
Member Author

The EpollSocketHalfClosedTest.testHalfClosureOnlyOneEventWhenAutoRead failures are not related to the changes in this PR, so I'm not letting that block merging.

@normanmaurer normanmaurer merged commit 9daa15f into netty:4.2 May 7, 2024
@normanmaurer normanmaurer added this to the 4.2.0.Final milestone May 7, 2024
@chrisvest chrisvest deleted the 4.2-method-handles branch May 7, 2024 13:22
@chrisvest chrisvest mentioned this pull request Jun 18, 2024
chrisvest added a commit to chrisvest/netty that referenced this pull request Jun 19, 2024
Motivation:
In netty#14032 we configured Graal to prepare reflective access to all public methods on the jdk.internal.misc.Unsafe.
This apparently includes linking to the getUncompressedObject method, which I think only exist in HotSpot and not in Graal, so the build fails with a linkage error.

Modification:
Only prepare reflective access for the methods we'll actually call.

Result:
No more Graal linkage errors during the build.
normanmaurer pushed a commit that referenced this pull request Jun 19, 2024
Motivation:
In #14032 we configured Graal to
prepare reflective access to all public methods on the
jdk.internal.misc.Unsafe.
This apparently includes linking to the getUncompressedObject method,
which I think only exist in HotSpot and not in Graal, so the build fails
with a linkage error.

Modification:
Only prepare reflective access for the methods we'll actually call.

Result:
No more Graal linkage errors during the build.
Spasi added a commit to Spasi/netty that referenced this pull request Mar 17, 2025
Motivation:
The code that detects the availability of allocateUninitializedArray has
been failing since netty#14032, because MethodHandle::invokeExact requires
exact type matches.

Modification:
Cast the return value to Object, matching the exact signature of
allocateUninitializedArray. The existing cast to byte[] remains and
happens immediately after the invocation.

Result:
Fixes netty#14930
normanmaurer pushed a commit that referenced this pull request Mar 18, 2025
Motivation:

The code that detects the availability of `allocateUninitializedArray`
has been failing since #14032, because `MethodHandle::invokeExact`
requires exact type matches.

Modification:

Cast the return value to `Object`, matching the exact signature of
`allocateUninitializedArray`. The existing cast to `byte[]` remains and
happens immediately after the invocation.

Result:

Fixes #14930
normanmaurer pushed a commit that referenced this pull request Mar 18, 2025
Motivation:

The code that detects the availability of `allocateUninitializedArray`
has been failing since #14032, because `MethodHandle::invokeExact`
requires exact type matches.

Modification:

Cast the return value to `Object`, matching the exact signature of
`allocateUninitializedArray`. The existing cast to `byte[]` remains and
happens immediately after the invocation.

Result:

Fixes #14930
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.

Replace reflection with method handles

3 participants