-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Require JDK Vector API at runtime #27340
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
Conversation
f28853c to
f6b0d0d
Compare
core/trino-main/src/main/java/io/trino/server/TrinoSystemRequirements.java
Outdated
Show resolved
Hide resolved
| such as Java 8, Java 11, Java 17, Java 21 or Java 23 do not work. | ||
| Newer versions such as Java 25 are not supported -- they may work, but are not tested. | ||
|
|
||
| Trino requires enabling the [JDK Vector API extensions](https://openjdk.org/jeps/489) at runtime. Code using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed. The launcher enables it automatically, so there's nothing for users to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall I rephrase this to make it clear the launcher already sets that flag, or just remove the section altogether? I've rephrased this section to make it more clear that the launcher sets the flag, but am happy to remove it altogether if you'd prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove it altogether. It's a detail users shouldn't be concerned with -- we handle everything that's needed under the covers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is good idea to mention it because we should showcase that we're actually using some cool new Java features. Also, developers launch Trino process without going through trino launcher and the information about adding the vector module as a requirement will avoid some confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which developer section should I add a mention of this to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhere in here: https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md, which is linked from https://trino.io/docs/current/develop.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If launcher is used, vector is linked by default: https://github.com/airlift/launcher/blob/master/src/main/go/args/java_static_config.go#L8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will follow up with docs in a subsequent PR. The relevant bits to include in DEVELOPMENT.md might merit some additional discussion that isn't necessary to block this PR on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #27342
8f2fe8e to
2aeb206
Compare
core/trino-main/src/main/java/io/trino/server/TrinoSystemRequirements.java
Outdated
Show resolved
Hide resolved
2aeb206 to
b5d882c
Compare
Do you know why it was done so? |
Presumably because we wanted to fallback gracefully when the API was not enabled (it's an incubating API, after all) |
|
@findepi I think that we assumed at that time, that we don't want to require vector API as we are unsure of its stability. Since then, it proved stable and the only reason this is incubating is its reliance on the Valhalla. |
|
It was from the days where we required Java 17 as the minimum version, and before we started tracking the latest JVM version. There was no canonical version to target. So, it was really an optional feature at the time. |
Adds a runtime check to ensure the Vector API is enabled on the current JVM during system startup.
Now that the vector API is a hard requirement, we can avoid probing for its presence based on reflection.
b5d882c to
39664a5
Compare
Description
Makes the JDK Vector API extensions (JEP 508) required by Trino at runtime.
Before this change, code in the parquet reader would attempt to discover whether the API was enabled via reflection and gracefully fall back to scalar code when not found at runtime. This worked, but required any SIMD implementations to be kept fully isolated such that SIMD related code would only be loaded at runtime by the JVM if enabled which is onerous to maintain as we add more SIMD logic throughout the engine. Requiring this API to be enabled at runtime allows us to write code that need not use reflection tricks and separate classes to detect and isolate SIMD implementations from scalar code.
Additional context and related issues
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: