Open franz1981 opened 1 month ago
@geoand and @vietj
This is a call stack which shows that memset is happening while it shouldn't - and the allocation of heap buffers in the hot path.
👌
This is from an old Norman M presentation but I can say that is still very relevant - I have tested OpenJDK 21 before opening this issue
I have verified that
allocating "big enough" (> 1024 by default) byte[] without zeroing them, using the "hidden" feature explained at https://shipilev.net/jvm/anatomy-quarks/7-initialization-costs/#_experiment. It can be enabled via -Dio.netty.tryReflectionSetAccessible=true --add-opens=java.base/jdk.internal.misc=ALL-UNNAMED
Is doing its job and helping, but sadly given that the benchmark I have used is limited by the database, it was "just" saving CPU cycles due to saving the zeroing, which is still a free lunch
The OpenSSL engine would be an issue in native mode.
Maybe? I remember some of the Netty stuff has been moved into native image - for micronaut. Maybe worth checking, and not only...what if we could enable it just on JVM mode? Or at least give the users the option to set it? For native transport we do already - and native cannot use it afaik
Maybe worth checking
Definitely
what if we could enable it just on JVM mode
This is something we generally shy away from doing as it compromised the native image experience.
Let's include @cescoffier into the conversation
I have always stayed away from the OpenSSL engine:
I can understand why we don't want to make OpenSSL enabled by default or through some discovery, but similarly to preferNativeTransport exposing it to users which knows what to do seems relevant, especially given the big difference in performance it could bring.
Conversely, to improve what we got we still have the option of adding -Dio.netty.tryReflectionSetAccessible=true --add-opens=java.base/jdk.internal.misc=ALL-UNNAMED
to the JVM args to at least reduce (a bit) the cost of allocating big byte[] in the hot path of JDK SSL.
Just for reference, before -Dio.netty.tryReflectionSetAccessible=true --add-opens=java.base/jdk.internal.misc=ALL-UNNAMED
:
while, after applying it
The violet part is io/netty/util/internal/PlatformDependent.allocateUninitializedArray
which cost is much reduced and the allocations which appear there are likely the ones before the default threshold of 1024 bytes.
@zakkak as well
Another option could be to enable pooling of byte[], but TBH I'm not feeling very happy about it and I beleive @vietj got some good reason why is not enabled by default on vertx, am I right?
I think we should make it easy for users to opt-in to OpenSSL because as @franz1981 has demonstrated, the performance improvement is non trivial
We could add support for OpenSSL. There would be a few action items and constraints:
I'm also concerned with the --add-opens popping up there and there...
I'm also concerned with the --add-opens popping up there and there...
AFAIU, this is not needed for OpenSSL
@cescoffier
+100
AFAIU, this is not needed for OpenSSL
Exactly: OpenSSL just use off-heap pooled buffers, which are fine as they are.
The JVM arg instead is key with JDK SSL to have decent performance - and still the allocations of byte[] won't go away for JDK SSL - which will likely impact heap sizing/RSS regardless.
So, about https://github.com/quarkusio/quarkus/issues/41880#issuecomment-2227851018: adding -Dio.netty.tryReflectionSetAccessible=true --add-opens=java.base/jdk.internal.misc=ALL-UNNAMED
to our JVM args. wdyt? we already have something similar I think.
So, about #41880 (comment): adding
-Dio.netty.tryReflectionSetAccessible=true --add-opens=java.base/jdk.internal.misc=ALL-UNNAMED
to our JVM args. wdyt? we already have something similar I think.
Adding --add-opens
cannot be done by us
I've proceed into understanding why such big allocations for JDK SSL and found that https://github.com/netty/netty/blob/5085cef149134951c94a02a743ed70025c8cdad4/handler/src/main/java/io/netty/handler/ssl/SslHandler.java#L334
handler.engine.getSession().getPacketBufferSize()
is 16709 bytes (!!) for SSLEngine
- which means that regardless the traffic or whatever, we always allocate such a big buffer(s).
@geoand I would like to reconsider what found in vertx at https://github.com/eclipse-vertx/vert.x/issues/5168#issuecomment-2258173211 and ask @vietj if we can enable heap buffer pooling if SSL is configured. Allocating such a big heap buffers in the hot path is really not what we want...
See https://github.com/netty/netty/commit/39cc7a673939dec96258ff27f5b1874671838af0#diff-963dd9b8e77e913395273812889e9b6f2f449fe55e2b748b95311b219d1c58faR302 for more info
https://github.com/netty/netty/issues/9685#issuecomment-544157904 too (although it was opened when off heap buffers were used)
I'v sent an additional commit to https://github.com/eclipse-vertx/vert.x/pull/5262 to address this . This is going to impact RSS, clearly, because the heap arena pools are not free and will stay still, regardless, per each event loop which is going to use it, including the thread-local ones, but the runtime performance will improve much more than just what shown https://github.com/quarkusio/quarkus/issues/41880#issuecomment-2227851018
I fear that https://github.com/netty/netty/pull/9696 is no longer valid, so i've opened https://github.com/netty/netty/issues/14208 to investigate on it
Sadly it seems we have no other choice than including other SSL engines somehow or at least proceed with https://github.com/eclipse-vertx/vert.x/pull/5262; https://github.com/netty/netty/issues/14208 shows that the JDK SSL engine still relies on byte[] and cannot use off-heap buffers proficiently.
Description
Currently, due to https://github.com/quarkusio/quarkus/issues/39907 the default JDK provider is (further) under performing. In particular:
And is pretty relevant in some profiling data on SSLEngine. To completely solve this, quarkus could expose setting, at least for JVM mode, the choice of the SSL engine which piggyback to https://vertx.io/docs/vertx-core/java/#_ssl_engine
The OpenSSL engine doesn't require to use heap Netty buffers, saving the needy to add JVM options to skip zeroing them.
This would make many JVM Quarkus users happier
Implementation ideas
No response