Skip to content
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

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

Open
olsavmic opened this issue Aug 29, 2024 · 6 comments
Open

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

olsavmic opened this issue Aug 29, 2024 · 6 comments

Comments

@olsavmic
Copy link

olsavmic commented Aug 29, 2024

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
Copy link
Author

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
Copy link
Contributor

ppalaga commented Aug 29, 2024

@gnodet do you remember, why we have that limit? I see it is supposed to solve #433 and #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
Copy link
Contributor

gnodet commented Aug 29, 2024

We can raise the limit, but we need to raise the global registry size limit too:

private static final int MAX_LENGTH = 32768;

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
Copy link
Contributor

ppalaga commented Aug 30, 2024

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
Copy link
Author

olsavmic commented Aug 30, 2024

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
Copy link
Contributor

ppalaga commented Aug 30, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants