-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
native compilation bring in unexpected Netty classes #42903
Comments
/cc @cescoffier (netty), @jponge (netty), @zakkak (native-image) |
@zakkak I would like to know if my analysis is correct and why we have such classes in |
@franz1981 can you clarify how you obtain this list? Are these classes loaded at build time? Do they end up in the image as well? Thanks |
and opened
which means that maybe I've not done the right thing, so please correct me 🙏 |
Thanks for the clarification @franz1981. So looking at it my understanding is that you expect GraalVM to figure out at build time whether unsafe is available or not, however that seems to not always be the case.
The interesting part is that Now irrespective of the above, I believe that GraalVM correctly includes both classes in the native executable since it doesn't know at build time which one will be needed at run-time, e.g. the user might set Note that this doesn't necessarily mean that both classes are indeed used at run-time, they are just available in case the user runs in a configuration where unsafe for some reason is not available. |
Yep, but that means likely, that all the call sites which receiver is not known at compile time, become bi or mega morphic. And that would likely prevent inlining too. I can verify it inspecting the compiled blobs (need to ask @galderz how) - but, what would be the right way to make graal able to prune the unused ones? |
Or just include an if-else :)
Why though? Sure it might not be optimal but inlining can still be performed.
Not sure if @galderz has a better way but I would suggest compiling with debug info generation enabled (
In this case I believe we would have to stop (re)initializing the whole |
In this case, let me check what I could see by using perf report on an actual benchmark - if lucky it will still enable us to observe how the actual buffer ops are compiled - or use gdb. |
That's assuming we cannot change Netty, while we can instead (if is acceptable and useful to others, clearly). But clearly this work should be justidied by:
|
Sorry @zakkak I have reread your comment and better understood
But currently I expect us to not "support" the case of noUnsafe (which was meant to exist mostly for Android-ish platforms really, and others JDK impls which were not providing it). |
I can help with that:I think I could split the unsafe presence check in a separate holder class within Netty AND other subsequent ones (bit-ness, unaligned support, max direct memory...) elsewhere. Let's sync together so I can better understand your concerns about portability, so we can drive how to split the detections. The more we can move it at build time, the better, I think |
AFAIK no user ever requested it and it was disabled for quite a long time (by having
I agree, that sounds like a good plan. |
@franz1981 I did some more digging into this and I see that:
To sum up, I see some differences in what's being brought in with the following patch: diff --git a/extensions/netty/runtime/src/main/java/io/quarkus/netty/runtime/graal/NettySubstitutions.java b/extensions/netty/runtime/src/main/java/io/quarkus/netty/runtime/graal/NettySubstitutions.java
index 37a79003cd5..e25189b27f6 100644
--- a/extensions/netty/runtime/src/main/java/io/quarkus/netty/runtime/graal/NettySubstitutions.java
+++ b/extensions/netty/runtime/src/main/java/io/quarkus/netty/runtime/graal/NettySubstitutions.java
@@ -621,6 +621,44 @@ final class Target_io_netty_util_internal_shaded_org_jctools_util_UnsafeRefArray
public static int LONG_ELEMENT_SHIFT;
}
+@TargetClass(className = "io.netty.util.internal.PlatformDependent")
+final class Target_io_netty_util_internal_PlatformDependent {
+
+ @Substitute
+ public static boolean hasUnsafe() {
+ return true;
+ }
+
+ @Substitute
+ public static Throwable getUnsafeUnavailabilityCause() {
+ return null;
+ }
+
+ @Substitute
+ public static boolean useDirectBufferNoCleaner() {
+ return true;
+ }
+}
+
+@TargetClass(className = "io.netty.util.internal.PlatformDependent0")
+final class Target_io_netty_util_internal_PlatformDependent0 {
+
+ @Substitute
+ static boolean hasUnsafe() {
+ return true;
+ }
+
+ @Substitute
+ static Throwable getUnsafeUnavailabilityCause() {
+ return null;
+ }
+
+ @Substitute
+ static boolean hasDirectBufferNoCleanerConstructor() {
+ return true;
+ }
+}
+
class IsBouncyNotThere implements BooleanSupplier {
@Override but I still see classes you originally mentioned you expect to not be present and it's not caused by some inaccuracy or uncertainty at build time, but due to class hierarchy in netty. |
thanks for checking @zakkak I didn't looked at the (weird) choice of class hiearchy of Netty :/ but the most important one seems valid, which you checked already i.e.
Now need to check this impact the quality of the generated assembly (and performance) I suspect that alignments is another one which we really don't expect to change - but I need to verify because it depends by the OS as well what happen if you don't do it |
@franz1981 and I have been looking at the assembly with the debugger to see if we can if megamorphic calls are being used to call into some of crucial We've focused on
We know that Also, we know that the assembly calls into
With both of those in mind and viewing the assembly from a few instructions before we see this:
The |
@galderz at this point, as @zakkak said; if we're "lucky" we should have just a bimorphic call there, but the fact that we have a |
Any progress on this one? |
@cescoffier Nothing to report |
Description
Compiling natively https://github.com/franz1981/quarkus-profiling-workshop produces a list of different recheable classes for Netty, including these:
which include some "unexpected" (to me at least) mutual exclusive variant o the same e.g.:
io.netty.buffer.PooledUnsafeDirectByteBuf
are mutually exclusive toio.netty.buffer.UnpooledDirectByteBuf
: see https://github.com/netty/netty/blob/95d86bbcee4f8e5a7d273d7ee16f69982cf2fab1/buffer/src/main/java/io/netty/buffer/PoolArena.java#L720-L722io.netty.buffer.UnpooledUnsafeDirectByteBuf
/io.netty.buffer.UnpooledUnsafeNoCleanerDirectByteBuf
andio.netty.buffer.UnpooledDirectByteBuf
: see https://github.com/netty/netty/blob/95d86bbcee4f8e5a7d273d7ee16f69982cf2fab1/buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java#L406-L407 and https://github.com/netty/netty/blob/4.1/buffer/src/main/java/io/netty/buffer/UnsafeByteBufUtil.java#L682-L687The same thing should applies to the
Heap
variants e.g.io.netty.buffer.PooledUnsafeHeapByteBuf
vsio.netty.buffer.PooledHeapByteBuf
- but I need to better understand if vetx is messing up with them.FYI these:
are still related the heap unpooled variant and should be cut in half depending if unsafe is available or not, see:
https://github.com/netty/netty/blob/95d86bbcee4f8e5a7d273d7ee16f69982cf2fab1/buffer/src/main/java/io/netty/buffer/UnpooledByteBufAllocator.java#L92-L94
The most of these classes presence depends by
Cleaner
s andUnsafe
presence, which should be something known upfront (and stored, in Netty, instatic final
fields), hence I would expect to work.In addition and separately, I've opened long time ago netty/netty#13459 which should take care (after changing vertx to benefit of this) to save these too:
To me, apart from improvement in memory footprint, is the arity of virtual calls on buffer operations which would improve A LOT, preventing likely inlining - and that's why native image PGO seems to shine with Netty: having a single byte/short/int/long buffer operation to not be inlined and preprended by some (although predicatable) branch (i.e. types checks) will kill it's performance for buffer maniputaion scenarios - which is the very core of the internal of Netty protocols.
Implementation ideas
No response
The text was updated successfully, but these errors were encountered: