-
Notifications
You must be signed in to change notification settings - Fork 5k
Lower default memory limits for Gradle processes #7343
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
8191e54
to
ac0e3ca
Compare
This VM is only there to display some log messages by default and thus shouldn't need a lot of memory. There is the corner case of running the build directly inside the client VM with --no-daemon. In that case some users may have to adjust their GRADLE_OPTS environment variable to accomodate their project.
We have made lots of memory usage improvements to Gradle, so 512m will be enough for small-medium sized projects. The assumption here is that power users with large projects are much more likely to tweak these settings than peole just getting started. Also this change limits the metaspace on Java8+, so that memory leaks in plugins don't go unnoticed.
ac0e3ca
to
45afbc5
Compare
.Changing JVM settings for the client VM | ||
---- | ||
org.gradle.jvmargs=-Xmx2g -XX:MaxPermSize=256m -XX:+HeapDumpOnOutOfMemoryError -Dfile.encoding=UTF-8 | ||
JAVA_OPTS="-Xmx64 -XX:MaxPermSize=64m -XX:+HeapDumpOnOutOfMemoryError -Dfile.encoding=UTF-8" |
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.
It should be -Xmx64m
, right?
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.
Right.
File propertiesFile = new File(projectDir, "gradle.properties"); | ||
Properties properties = propertiesFile.exists() ? GUtil.loadProperties(propertiesFile) : new Properties(); | ||
String existingArgs = properties.getProperty(ORG_GRADLE_JVMARGS, ""); | ||
properties.setProperty(ORG_GRADLE_JVMARGS, "-Xmx1024m -XX:MaxMetaspaceSize=512m -XX:+HeapDumpOnOutOfMemoryError " + existingArgs); |
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 is a little odd - Our sample tests should have failed on Java 7 before if they can't deal with 256m of space for classes. The only reason I can come up with is that we switched to Exemplar after we removed Java 7 coverage.
In any case, we should find out why they take so much metaspace. They aren't very big or complex, so our normal threshold should be enough.
45afbc5
to
66a773d
Compare
5d55785
to
8abbcee
Compare
I'm moving the Metaspace limit into its own issue (#7385), as I suspect we've introduced a leak. I don't want to hold up the other parts of this PR though. |
8abbcee
to
775ab81
Compare
|
||
### Lower default memory settings | ||
|
||
The command line client now starts with 64m of heap instead of 1g. |
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'd be very helpful to include this in the upgrade_gradle_4.adoc.
Seems like it ought to get it's own bullet point in the preamble. Agreed @pledbrook?
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.
Indeed.
cc76b9b
to
91e49f9
Compare
Our workers for compilation, testing etc. had no default memory settings until now, which mean that we would use the default given by the JVM, which is 1/4th of system memory in most cases. This is way too much when running with a high number of workers. Limiting this to 256m should be enough for small to medium sized projects. Larger projects will have to tweak this value.
We used to test Gradle with a permgen limit of 320Mb. However, trying to enforce the same limit for metaspace on Java8+ consistently leads to OutOfMemoryErrors. Even raising it significantly to 1G does not seem to solve all these issues. The only explanation I currently have is that we introduced a leak between now and the point when we stopped testing for Java 7.
91e49f9
to
5539f7a
Compare
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.
@oehme When I bumped to the latest nightly, I found a weird thing with the gradlew scripts. Please see my comment here.
|
||
@rem Add default JVM options here. You can also use JAVA_OPTS and GRADLE_OPTS to pass JVM options to this script. | ||
set DEFAULT_JVM_OPTS=-Xmx128m -Dfile.encoding=UTF-8 | ||
set DEFAULT_JVM_OPTS=-Xmx64m -Dfile.encoding=UTF-8 |
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.
When I generate a new wrapper with the latest nightly on release now, I get this generated in gradlew.bat:
set DEFAULT_JVM_OPTS=-Xmx128m -Dfile.encoding=UTF-8"-Xmx64m"
And this in gradlew
:
DEFAULT_JVM_OPTS='"-Xmx64m"'
The gradlew script seems to work. I haven't tried the gradlew.bat script, but it looks wrong.
I think DEFAULT_JVM_OPTS in gradlew script shouldn't be singled quoted.
I think the problems with our wrapper script are self-inflicted somewhere, maybe WrapperPlugin.kt
?
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.
Interesting, I didn't change anything about the quoting, so if this is a problem it would have existed in our script generator for a long time.
The superflous Xmx is from WrapperPlugin.kt. We can just remove that.
### Lower default memory settings | ||
|
||
The command line client now starts with 64m of heap instead of 1g. | ||
This may affect builds running directly inside the client VM using --no-daemon mode. |
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.
Should this also mention org.gradle.daemon=false
?
Fixes #6216