Add -XX:+UnlockDiagnosticVMOptions and -XX:+UseAESCTRIntrinsics

[radek-kondziolka]

Description

This PR adds options -XX:+UnlockDiagnosticVMOptions and -XX:+UseAESCTRIntrinsics to the core/docker/default/etc/jvm.config and core/trino-server-rpm/src/main/resources/config/jvm.config to enable intrinsic support for AES CTR/GCM on ARM64.
This is motivated by the SSL communication overhead observed with S3.

Is this change a fix, improvement, new feature, refactoring, or other?

It is a kind of improvement.

How would you describe this change to a no

n-technical end user or system administrator?

We make cryptography stuff (TLS) faster on ARM64 architectures by configuration JVM.

Documentation

( ) No documentation is needed.
( x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

I do not know whether we should put that information in release notes.

TPC(DS/H) benchmarks result:

tpcds_tpch_benchmark.pdf

[raunaqmorarka]

We should update the jvm.config in product tests and deployment.rst as well

[raunaqmorarka]

Also, please share TPC benchmark results here

[radek-kondziolka]

@sopel39 , your comments were addressed.

[findepi]

#12253 links to openjdk/jdk11u-dev#410 where openjdk/jdk11u-dev#410 (review) may indicate why it is off by default.

is it safe to turn on for all Trino users?
does it affect ARM64 only?

[sopel39]

@findepi
There is this comment openjdk/jdk11u-dev#410 (comment)
Java 18 is already released (in March) with this feature on by default, so I thinking this should be relatively safe to merge (also we've benchmarked it quite thoroughly)

does it affect ARM64 only?

I think so, but please @radek-starburst verify.

[radek-kondziolka]

@findepi
-XX:+UnlockDiagnosticVMOptions, -XX:+UseAESCTRIntrinsics as you know are global in the sense of JVM so they are applied, obviously, regardless of a specific architecture. The -XX:+UseAESCTRIntrinsics is a diagnostic option. This is why we have to enable UnlockDiagnosticVMOptions to use the previous one.

This change is targeted for ARM64 architecture because the backport was targeted to ARM64. We do not see a performance problem of AES encryption on x86 - what is in line with we can read in a backport PR and issue: https://bugs.openjdk.java.net/browse/JDK-8271567. Moreover, I checked locally the status of flag UseAESCTRIntrinsics on our docker images:

• arm64: disabled by default
• amd64: enabled by default

This flag was disabled by default in a backport because of conservative approach (openjdk/jdk11u-dev#410).

Moreover, after enabling -XX:+UseAESIntrinsics we see performance gain (~8% CPU time on TPCH) and (~5% CPU time on TPCDS).

According to: https://docs.oracle.com/javase/8/docs/technotes/tools/unix/java.html

UseAES and UseAESIntrinsics flags are enabled by default and are supported only for Java HotSpot Server VM 32-bit and 64-bit. To disable hardware-based AES intrinsics, specify -XX:-UseAES -XX:-UseAESIntrinsics. For example, to enable hardware AES, use the following flags:

-XX:+UseAES -XX:+UseAESIntrinsics

To support UseAES and UseAESIntrinsics flags for 32-bit and 64-bit use -server option to choose Java HotSpot Server VM. These flags are not supported on Client VM.

we just enable AES intrinsics, that are enabled on x86 by default and newer Java versions enable them default as well. So, on my eye, it is safe to enable it.

[electrum]

We’re about to update to JDK 17. If thus is only needed for the small set of users who are (1) on ARM64 and (2) using secure internal communication, then we shouldn’t add clutter and churn.

The documentation needs work as well, since this doesn’t affect most users. If we really cared, it could go on the secure internal communication page as a small note.

[raunaqmorarka]

We’re about to update to JDK 17. If thus is only needed for the small set of users who are (1) on ARM64 and (2) using secure internal communication, then we shouldn’t add clutter and churn.

The documentation needs work as well, since this doesn’t affect most users. If we really cared, it could go on the secure internal communication page as a small note.

@electrum This was motivated by the SSL communication overhead observed with S3 (hive.s3.ssl.enabled is enabled by default). Although secure internal comms might benefit from it, we didn't make this change for that.
Also, upgrading to Java 17 will not solve it because the fix is enabled by default only in Java 18 at the moment. Though this might change in future Java 17 releases.
https://bugs.openjdk.java.net/browse/JDK-8271567
openjdk/jdk17u#216

[electrum]

This is the first mention of S3. If that’s the primary motivation, why is the PR description so vague?

Drop the PR links and remove the TODOs everywhere as they add clutter and look bad. Simplify the description everywhere to

Improve AES performance for S3, etc. on ARM64 (JDK-8271567)

The extra details aren’t useful to end users, especially in the documentation. The JDK bug number is sufficient to Google or for anyone who cares about this sort of thing, without looking junky. Anyone else won’t care.

[radek-kondziolka]

This is the first mention of S3. If that’s the primary motivation, why is the PR description so vague?

Drop the PR links and remove the TODOs everywhere as they add clutter and look bad. Simplify the description everywhere to

Improve AES performance for S3, etc. on ARM64 (JDK-8271567)

The extra details aren’t useful to end users, especially in the documentation. The JDK bug number is sufficient to Google or for anyone who cares about this sort of thing, without looking junky. Anyone else won’t care.

@electrum
Your suggestions were adressed. There are still links to PR / issue in the commit message, is it ok for you?

[losipiuk]

LGTM.
As for the release notes you can maybe go with sth like:

Update default JVM options to improve network communication performance that uses AES on ARM64. Applicable to communication with S3 and others.

[electrum]

Looks much better now. Thanks for making these changes.