apache / maven-mvnd

Apache Maven Daemon
https://maven.apache.org/
Apache License 2.0
2.92k stars 212 forks source link

Too many JVM options resulting in no-reuse of daemons #1119

Open olsavmic opened 2 months ago

olsavmic commented 2 months ago

Given the following artificially constructed .mvn/jvm.config, the daemons are NEVER reused.

--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED
--add-opens java.base/java.util.concurrent=ALL-UNNAMED

Removing a single line in this config immediately results in correct daemon reuse. We have discovered that it does not matter what JVM options are used, it's only related to the total length of options.

(Running on MacOS, Apple Silicon, JDK 22 Temurin, mvnd 1.0.2)

olsavmic commented 2 months ago

The root cause is that options are capped at 1024 bytes in org.mvndaemon.mvnd.common.DaemonRegistry#writeString and the fact that all JVM options are passed as a single JVM options param.

ppalaga commented 2 months ago

@gnodet do you remember, why we have that limit? I see it is supposed to solve https://github.com/apache/maven-mvnd/issues/433 and https://github.com/apache/maven-mvnd/issues/432 but it is not obvious why the limit is 1024. We could allow up to Short.MAX_VALUE, no? Or even store the length as int.

gnodet commented 2 months ago

We can raise the limit, but we need to raise the global registry size limit too: https://github.com/apache/maven-mvnd/blob/50653ae7d92badef15d42151b7df3f827f9a8678/common/src/main/java/org/mvndaemon/mvnd/common/DaemonRegistry.java#L59 The registry should be kept small and not used to store random strings, as it's kept in memory. That's why the only "long" string was cut I think (it's usually the stop reason). I wonder if storing a digest of the JVM properties would be sufficient...

ppalaga commented 2 months ago

storing a digest of the JVM properties would be sufficient.

That would solve the problem, but I am not sure the perf penalty on every mvnd invocation is worth of it? After all, 99% of builds have none or just a few props. I'd say the memory penalty should be rather paid by folks having many props.

@olsavmic I wonder, what would be the limit in bytes that would solve this for you?

olsavmic commented 2 months ago

private static final int MAX_LENGTH = 32768;

Looking at the implementation, it actually seems that the constant is not named correctly. The code already supports resizing of the buffer based on the needs (both up and down). The MAX_LENGTH is just a default AND min value.

@ppalaga

@olsavmic I wonder, what would be the limit in bytes that would solve this for you?

From the practical standpoint, I doubt we will ever go over 2x the current limit. We're just slightly over the limit in one of our projects due the --add-opens requirements of the static analysis libs we're using.

Given the auto-resizing buffer logic, I'd argue that switching to Short.MAX_VALUE is a good solution for all real-world projects.

ppalaga commented 2 months ago

Sounds good to me. Would you like to send a PR with a test?