Skip to content

Use different G1GC options for small heaps #59667

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.

Sign up for GitHub

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

Merged
merged 23 commits into from
Oct 6, 2020
Merged

Use different G1GC options for small heaps #59667

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
7d987ba
Use the parallel collector for small heaps
ebadyano Jul 15, 2020
ab6059d
Merge branch 'master' of github.com:elastic/elasticsearch into master…
ebadyano Jul 28, 2020
2655315
Merge branch 'master' of github.com:elastic/elasticsearch into master…
ebadyano Jul 29, 2020
2003331
Addressed some comments and fixed tests.
ebadyano Jul 29, 2020
3c05949
Fix precomit erorrs
ebadyano Jul 29, 2020
818226a
Merge branch 'master' of github.com:elastic/elasticsearch into master…
ebadyano Jul 30, 2020
f14d2cd
Merge branch 'master' of github.com:elastic/elasticsearch into master…
ebadyano Jul 31, 2020
4d033a4
Fix test
ebadyano Jul 31, 2020
9008ede
Merge branch 'master' of github.com:elastic/elasticsearch into master…
ebadyano Sep 22, 2020
d0e34fe
Changed the ergonomics heuristic to use G1GC with tuned options instead
ebadyano Sep 29, 2020
70b4de9
Merge branch 'master' of github.com:elastic/elasticsearch into master…
ebadyano Sep 29, 2020
a6f5302
fix precomit errors
ebadyano Sep 29, 2020
ffec016
Merge branch 'master' of github.com:elastic/elasticsearch into master…
ebadyano Sep 29, 2020
80b713b
Fix precomit errors
ebadyano Sep 29, 2020
411cfd4
Fix indentation
ebadyano Oct 1, 2020
ec2b891
Addressed Henning's and Ryan's comments.
ebadyano Oct 2, 2020
8d3c734
Merge branch 'master' of github.com:elastic/elasticsearch into master…
ebadyano Oct 2, 2020
ac9a568
Address Henning's comments
ebadyano Oct 5, 2020
f326f5b
address Henning's comments
ebadyano Oct 5, 2020
5ddb619
Fix tests
ebadyano Oct 5, 2020
d628ff9
Fix precommit tests
ebadyano Oct 5, 2020
d8a3840
Fix logic
ebadyano Oct 5, 2020
f2e999a
Add an extra test for when non G1GC policy is set
ebadyano Oct 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions distribution/src/config/jvm.options
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,6 @@
# 8-13:-XX:-UseConcMarkSweepGC
# 8-13:-XX:-UseCMSInitiatingOccupancyOnly
14-:-XX:+UseG1GC
14-:-XX:G1ReservePercent=25
14-:-XX:InitiatingHeapOccupancyPercent=30

## JVM temporary directory
-Djava.io.tmpdir=${ES_TMPDIR}
Expand Down
74 changes: 65 additions & 9 deletions ...bution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/JvmErgonomics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,25 +53,57 @@ private JvmErgonomics() {
*/
static List<String> choose(final List<String> userDefinedJvmOptions) throws InterruptedException, IOException {
final List<String> ergonomicChoices = new ArrayList<>();
final Map<String, Optional<String>> finalJvmOptions = finalJvmOptions(userDefinedJvmOptions);
final Map<String, JvmOption> finalJvmOptions = finalJvmOptions(userDefinedJvmOptions);
final long heapSize = extractHeapSize(finalJvmOptions);
final long maxDirectMemorySize = extractMaxDirectMemorySize(finalJvmOptions);
if (maxDirectMemorySize == 0) {
ergonomicChoices.add("-XX:MaxDirectMemorySize=" + heapSize / 2);
}
final boolean tuneG1GCForSmallHeap = tuneG1GCForSmallHeap(finalJvmOptions, heapSize);
final boolean tuneG1GCForLargeHeap = tuneG1GCForLargeHeap(finalJvmOptions, heapSize);
if (tuneG1GCForSmallHeap) {
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 is surprising that if you manually specify any of the 3 parameters you loose all 3. For instance if they decide to raise G1ReservePercent to 20, the region size adjustment is lost.

I think the check in tuneG1GCForSmallHeap should only check heap size and maybe that G1 is in use.

The code below should then only add options where there is no existing command line origin option.

Alternatively (to the last part), we could swap the two lines here:

elasticsearch/distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/JvmOptionsParser.java

Line 144 in 01dc110

finalJvmOptions.addAll(substitutedJvmOptions);

to be:

        finalJvmOptions.addAll(ergonomicJvmOptions);
        finalJvmOptions.addAll(substitutedJvmOptions);

since that would ensure that the original command line options override the ergonomics picked here.

ergonomicChoices.add("-XX:G1HeapRegionSize=4m");
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned on another channel, I think we should make reserve pct 15 rather than the default 10 - to stay safely clear of real memory circuit breaker exceptions.

I also think we should add in the IHOP always if it is not defined, just to be safe. It should not matter for performance since G1 will adaptively adjust the value.

ergonomicChoices.add("-XX:G1ReservePercent=15");
ergonomicChoices.add("-XX:InitiatingHeapOccupancyPercent=45");
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 we should use 30 in this case too.

} else if (tuneG1GCForLargeHeap) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above, is surprising that if one of them is specified you loose the other. In particular if IHOP is specified, it seems quite dangerous to no longer set G1ReservePercent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for catching this, I updated the pr.

ergonomicChoices.add("-XX:G1ReservePercent=25");
ergonomicChoices.add("-XX:InitiatingHeapOccupancyPercent=30");
}

return ergonomicChoices;
}

private static final Pattern OPTION = Pattern.compile(
"^\\s*\\S+\\s+(?<flag>\\S+)\\s+:?=\\s+(?<value>\\S+)?\\s+\\{[^}]+?\\}\\s+\\{[^}]+}"
"^\\s*\\S+\\s+(?<flag>\\S+)\\s+:?=\\s+(?<value>\\S+)?\\s+\\{[^}]+?\\}\\s+\\{(?<origin>[^}]+)}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Drive-by comment, feel free to ignore - should we pass Pattern.COMMENTS here so that we can break up the regex with whitespace, to make it easier to read?

);

static Map<String, Optional<String>> finalJvmOptions(final List<String> userDefinedJvmOptions) throws InterruptedException,
IOException {
private static class JvmOption {
private final String value;
private final String origin;

JvmOption(String value, String origin) {
this.value = value;
this.origin = origin;
}

public Optional<String> getValue() {
return Optional.ofNullable(value);
}

public String getMandatoryValue() {
return value;
}

public boolean isCommandLineOrigin() {
return "command line".equals(this.origin);
}
}

static Map<String, JvmOption> finalJvmOptions(final List<String> userDefinedJvmOptions) throws InterruptedException, IOException {
return flagsFinal(userDefinedJvmOptions).stream()
.map(OPTION::matcher)
.filter(Matcher::matches)
.collect(Collectors.toUnmodifiableMap(m -> m.group("flag"), m -> Optional.ofNullable(m.group("value"))));
.collect(Collectors.toUnmodifiableMap(m -> m.group("flag"), m -> new JvmOption(m.group("value"), m.group("origin"))));
}

private static List<String> flagsFinal(final List<String> userDefinedJvmOptions) throws InterruptedException, IOException {
Expand Down Expand Up @@ -116,12 +148,36 @@ private static List<String> readLinesFromInputStream(final InputStream is) throw
}

// package private for testing
static Long extractHeapSize(final Map<String, Optional<String>> finalJvmOptions) {
return Long.parseLong(finalJvmOptions.get("MaxHeapSize").get());
static Long extractHeapSize(final Map<String, JvmOption> finalJvmOptions) {
return Long.parseLong(finalJvmOptions.get("MaxHeapSize").getMandatoryValue());
}

static long extractMaxDirectMemorySize(final Map<String, JvmOption> finalJvmOptions) {
return Long.parseLong(finalJvmOptions.get("MaxDirectMemorySize").getMandatoryValue());
}

// Tune G1GC options for heaps < 8GB unless the user has explicitly set G1HeapRegionSize
static boolean tuneG1GCForSmallHeap(final Map<String, JvmOption> finalJvmOptions, final long heapSize) {
JvmOption g1GC = finalJvmOptions.get("UseG1GC");
JvmOption g1GCHeapRegion = finalJvmOptions.get("G1HeapRegionSize");
JvmOption g1GCReservePercent = finalJvmOptions.get("G1ReservePercent");
JvmOption g1GCInitiatingHeapOccupancyPercent = finalJvmOptions.get("InitiatingHeapOccupancyPercent");
return (heapSize < 8L << 30
&& g1GC.getMandatoryValue().equals("true")
&& g1GCHeapRegion.isCommandLineOrigin() == false
&& g1GCReservePercent.isCommandLineOrigin() == false
&& g1GCInitiatingHeapOccupancyPercent.isCommandLineOrigin() == false);
}

static long extractMaxDirectMemorySize(final Map<String, Optional<String>> finalJvmOptions) {
return Long.parseLong(finalJvmOptions.get("MaxDirectMemorySize").get());
// Tune G1GC options for heaps >= 8GB unless the user has explicitly set -XX:G1ReservePercent or -XX:InitiatingHeapOccupancyPercent
static boolean tuneG1GCForLargeHeap(final Map<String, JvmOption> finalJvmOptions, final long heapSize) {
JvmOption g1GC = finalJvmOptions.get("UseG1GC");
JvmOption g1GCReservePercent = finalJvmOptions.get("G1ReservePercent");
JvmOption g1GCInitiatingHeapOccupancyPercent = finalJvmOptions.get("InitiatingHeapOccupancyPercent");
return (heapSize >= 8L << 30
&& g1GC.getMandatoryValue().equals("true")
&& g1GCReservePercent.isCommandLineOrigin() == false
&& g1GCInitiatingHeapOccupancyPercent.isCommandLineOrigin() == false);
}

private static final Pattern SYSTEM_PROPERTY = Pattern.compile("^-D(?<key>[\\w+].*?)=(?<value>.*)$");
Expand Down
27 changes: 27 additions & 0 deletions ...n/tools/launchers/src/test/java/org/elasticsearch/tools/launchers/JvmErgonomicsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,33 @@ public void testExtractSystemProperties() {
assertEquals(expectedSystemProperties, parsedSystemProperties);
}

public void testG1GOptionsForSmallHeap() throws InterruptedException, IOException {
assertThat(JvmErgonomics.choose(Arrays.asList("-Xms6g", "-Xmx6g", "-XX:+UseG1GC")), hasItem("-XX:G1HeapRegionSize=4m"));
}

public void testG1GOptionsForSmallHeapWhenTuningSet() throws InterruptedException, IOException {
assertThat(
JvmErgonomics.choose(Arrays.asList("-Xms6g", "-Xmx6g", "-XX:+UseG1GC", "-XX:G1HeapRegionSize=4m")),
everyItem(not(startsWith("-XX:G1HeapRegionSize=")))
);
}

public void testG1GOptionsForLargeHeap() throws InterruptedException, IOException {
assertThat(
JvmErgonomics.choose(Arrays.asList("-Xms8g", "-Xmx8g", "-XX:+UseG1GC")),
hasItem("-XX:InitiatingHeapOccupancyPercent=30")
);
}

public void testG1GOptionsForLargeHeapWhenTuningSet() throws InterruptedException, IOException {
assertThat(
JvmErgonomics.choose(
Arrays.asList("-Xms8g", "-Xmx8g", "-XX:+UseG1GC", "-XX:InitiatingHeapOccupancyPercent=60", "-XX:G1ReservePercent=10")
),
everyItem(not(startsWith("-XX:InitiatingHeapOccupancyPercent=")))
);
}

public void testExtractNoSystemProperties() {
Map<String, String> parsedSystemProperties = JvmErgonomics.extractSystemProperties(Arrays.asList("-Xms1024M", "-Xmx1024M"));
assertTrue(parsedSystemProperties.isEmpty());
Expand Down