-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Enable the Panama Vector module #96453
Conversation
distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcess.java
Outdated
Show resolved
Hide resolved
distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcess.java
Outdated
Show resolved
Hide resolved
Pinging @elastic/es-search (Team:Search) |
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.
In addition to my comment on how to plumb the option, I have a thought on the warning:
The resolution of the vector module will raise a warning on startup: WARNING: Using incubator modules: jdk.incubator.vector. This warning and the property will be documented, as well the log output from Lucene regarding the vector bit width.
For the mmap warning, we filter the response, and I think we should here too so as not to confuse users (and it won't be necessary to document specially then?). Though the way we currently filter is a bit hacky (raw stderr filtering). See #90526 (comment), which should be fixable after we implement #94613.
distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ServerProcess.java
Outdated
Show resolved
Hide resolved
Hi @ChrisHegarty, I've created a changelog YAML for you. |
The incubator warning goes to stderr very early in startup. I think before we can redirect. E.g.
This does not help, e.g. --- a/server/src/main/java/org/elasticsearch/common/logging/LogConfigurator.java
+++ b/server/src/main/java/org/elasticsearch/common/logging/LogConfigurator.java
@@ -261,7 +261,7 @@ public class LogConfigurator {
// third party libraries may do that. Note that we do NOT close the streams because other code may have
// grabbed a handle to the streams and intend to write to it, eg log4j for writing to the console
System.setOut(
- new PrintStream(new LoggingOutputStream(LogManager.getLogger("stdout"), Level.INFO, List.of()), false, StandardCharsets.UTF_8)
+ new PrintStream(new LoggingOutputStream(LogManager.getLogger("stdout"), Level.INFO, List.of("WARNING: Using incubator modules: jdk.incubator.vector")), false, StandardCharsets.UTF_8)
);
System.setErr(
new PrintStream(
@@ -270,7 +270,7 @@ public class LogConfigurator {
Level.WARN,
// MMapDirectory messages come from Lucene, suggesting to users as a warning that they should enable preview features in
// the JDK
- List.of("MMapDirectory")
+ List.of("MMapDirectory", "WARNING: Using incubator modules: jdk.incubator.vector")
),
false,
StandardCharsets.UTF_8 Or is there some way of filtering through the connection with the startup CLI process ? |
Note: Lucene emits a log message - which we absolutely want - noting the preferred bit size that is in use for the hardware that we're running on. E.g. running on my mac I can see the following in the Elasticsearch logs:
I can confirm that this is currently the case. |
Thanks @ChrisHegarty ! As a follow-up, do we need to do anything on the mappings side to try and suggest / adjust the number of dimensions using even numbers that are friendly to the accelerated vector hardware instructions? |
This magic filters out the JDK's incubator warning from the output/logs, 3239e33 |
There are certainly some dimensions that perform better than others, but of course all dimension sizes behave correctly. E.g. 1536 will perform better than 1535. I added a note about this here: #96370 (comment) |
Hi @ChrisHegarty, I've updated the changelog YAML for you. |
List.of("MMapDirectory") | ||
// the JDK. Vector logs come from Lucene too, but only if the used explicitly disables the Vector API - no point warning | ||
// in this case. | ||
List.of("MMapDirectory", "VectorUtilProvider", "WARNING: Java vector incubator module is not readable") | ||
), |
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.
This suppresses the two line warning from Lucene, if one was to disable the Vector API (by editing jvm.options).
[2023-06-01T15:15:29,430][WARN ][stderr ] [yamlRestTest-0] Jun 01, 2023 3:15:29 PM org.apache.lucene.util.VectorUtilProvider lookup
[2023-06-01T15:15:29,434][WARN ][stderr ] [yamlRestTest-0] WARNING: Java vector incubator module is not readable. For optimal vector performance, pass '--add-modules jdk.incubator.vector' to enable Vector API.
distribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ErrorPumpThread.java
Show resolved
Hide resolved
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.
LGTM
@@ -68,6 +69,9 @@ void drain() { | |||
nonInterruptibleVoid(this::join); | |||
} | |||
|
|||
/** List of messages / lines to filter from the output. */ | |||
List<String> filter = List.of("WARNING: Using incubator modules: jdk.incubator.vector"); |
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.
nit: can this be moved to the top of the class and made private static final
?
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.
Sorry, only seen this after merging. I'll move this as a follow-up.
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.
LGTM
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.
LGTM
This change adds the jdk.incubator.vector module, so that we can enable the Panamaized vector utils in Lucene. apache/lucene#12311
The module is added by default if running on JDK 20, which is the only current supported implementation in Lucene, but JDK 21 will likely come soon.
If for some reason this needs to be disable, just remove or otherwise comment out the
--add-modules=jdk.incubator.vector
line in the jvm.options.The log output from Lucene shows the preferred vector bit width that is in operation.
relates #96370