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

Maven jvm.config instead of fork/compilerArgs #2786

Open
mattnelson opened this issue Dec 28, 2021 · 6 comments
Open

Maven jvm.config instead of fork/compilerArgs #2786

mattnelson opened this issue Dec 28, 2021 · 6 comments

Comments

@mattnelson
Copy link
Contributor

The error prone maven instructions for jdk16+1 recommend using compilerArgs and fork. I was able to supply those same args to maven itself via .mvn/jvm.config file2 and get a 4x improvement of build times by no longer forking the compiler.

Anecdotally, with my sample size of 1, using a maven reactor with 80 modules the build time for mvn clean compile was 11s using .mvn/jvm.config and 44s when forking the compiler.

The maven-compiler-plugin docs3 leads me to believe that fork was designed for use cases where a different JDK was wanting to be used, rather than for configuration isolation while using the default javac. Thats not to say it can't also work when config isolation is desired, like in this case.

Does errorprone need that level of isolation on the exports/opens configuration? Would the maintainers be open to updating the docs to include the .mvn/jvm.config option with the pros/con between the two approaches? or if the config isolation is not needed and the performance impact is impactful enough, change the recommendation to the non-forking option.

Footnotes

  1. https://errorprone.info/docs/installation#jdk-16

  2. https://maven.apache.org/configure.html#mvn-jvm-config-file

  3. https://maven.apache.org/plugins/maven-compiler-plugin/examples/compile-using-different-jdk.html

@cushon
Copy link
Collaborator

cushon commented Dec 28, 2021

Thanks for the suggestion! Using .mvn/jvm.config sounds like a good approach. We aren't relying on fork for isolation, it's really just about having a way to pass the JVM flags to javac, and passing -J flags to javac only works with fork enabled.

I'm trying this out on a couple of projects and so far everything looks good.

Any interest in sending a PR to update the docs to suggest .mvn/jvm.config instead of fork?

@mattnelson
Copy link
Contributor Author

Any interest in sending a PR to update the docs to suggest .mvn/jvm.config instead of fork?

Sure, PR issued at #2788

copybara-service bot pushed a commit to google/google-java-format that referenced this issue Dec 30, 2021
copybara-service bot pushed a commit to google/turbine that referenced this issue Dec 30, 2021
copybara-service bot pushed a commit to google/google-java-format that referenced this issue Dec 30, 2021
copybara-service bot pushed a commit to google/turbine that referenced this issue Dec 30, 2021
@hisener
Copy link
Contributor

hisener commented Dec 31, 2021

get a 4x improvement of build times by no longer forking the compiler.

@mattnelson Funnily enough, I applied the exact change on Tuesday, and the build times went from ~20 minutes to 8-9 minutes. However, the downside is that the Metaspace usage went up because the number of classes loaded increased significantly. Did you experience the same?


it's really just about having a way to pass the JVM flags to javac, and passing -J flags to javac only works with fork enabled.

@cushon By looking at the maven-compiler-plugin code, I suspect the following description misleading:

Sets the arguments to be passed to the compiler if fork is set to true.
https://maven.apache.org/plugins/maven-compiler-plugin/compile-mojo.html#compilerArgs

Should we file an MCOMPILER ticket? 🤔

@cushon
Copy link
Collaborator

cushon commented Jan 3, 2022

Should we file an MCOMPILER ticket?

Sure, if you're willing to follow up on that, it sounds worth clarifying. I verified that I'm still seeing the behaviour where -J options only work with <fork>true</fork>. I think that's understandable, since javac passes them through as VM options, and without fork enabled it's too late to change VM flags. But perhaps that could be at least clarified in the docs, or maven could report an error if -J is set without fork enabled, or something.

@mattnelson
Copy link
Contributor Author

Funnily enough, I applied the exact change on Tuesday, and the build times went from ~20 minutes to 8-9 minutes. However, the downside is that the Metaspace usage went up because the number of classes loaded increased significantly. Did you experience the same?

I was not forking the compiler before upgrading to 17, so whatever was used before was within my limits.

@hisener
Copy link
Contributor

hisener commented Jan 4, 2022

Filed https://issues.apache.org/jira/browse/MCOMPILER-479 and #2793.

copybara-service bot pushed a commit that referenced this issue Jan 27, 2022
Doc updates from #2786

Fixes #2788

COPYBARA_INTEGRATE_REVIEW=#2788 from mattnelson:gh-pages c5f98dc
PiperOrigin-RevId: 424710688
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