Skip to content

Conversation

oehme
Copy link
Contributor

@oehme oehme commented Oct 11, 2018

Fixes #6216

@oehme oehme added this to the 5.0 RC1 milestone Oct 11, 2018
@oehme oehme self-assigned this Oct 11, 2018
@oehme oehme requested a review from marcphilipp October 11, 2018 15:54
@oehme oehme changed the base branch from master to release October 11, 2018 15:54
@oehme oehme force-pushed the oehme/memory-limits branch 2 times, most recently from 8191e54 to ac0e3ca Compare October 11, 2018 15:59
oehme added 2 commits October 11, 2018 17:59
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.
@oehme oehme force-pushed the oehme/memory-limits branch from ac0e3ca to 45afbc5 Compare October 11, 2018 16:00
.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"
Copy link
Contributor

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?

Copy link
Contributor Author

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);
Copy link
Contributor Author

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.

@oehme oehme force-pushed the oehme/memory-limits branch from 45afbc5 to 66a773d Compare October 12, 2018 08:26
@oehme oehme force-pushed the oehme/memory-limits branch 2 times, most recently from 5d55785 to 8abbcee Compare October 15, 2018 13:35
@oehme
Copy link
Contributor Author

oehme commented Oct 15, 2018

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.

@oehme oehme force-pushed the oehme/memory-limits branch from 8abbcee to 775ab81 Compare October 15, 2018 14:29

### Lower default memory settings

The command line client now starts with 64m of heap instead of 1g.
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed.

@oehme oehme force-pushed the oehme/memory-limits branch from cc76b9b to 91e49f9 Compare October 16, 2018 09:16
oehme added 7 commits October 16, 2018 12:16
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.
@oehme oehme force-pushed the oehme/memory-limits branch from 91e49f9 to 5539f7a Compare October 16, 2018 10:16
@oehme oehme merged commit 030ca9e into release Oct 18, 2018
@oehme oehme deleted the oehme/memory-limits branch October 18, 2018 07:17
Copy link
Member

@big-guy big-guy left a 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
Copy link
Member

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?

Copy link
Contributor Author

@oehme oehme Oct 21, 2018

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.
Copy link
Member

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?

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

Successfully merging this pull request may close these issues.

4 participants