Use different G1GC options for small heaps #59667

ebadyano · 2020-07-15T20:50:17Z

Our benchmarks have demonstrated that Elasticsearch performs better with -XX:G1HeapRegionSize=4M and -XX:G1ReservePercent=15 options for the small heap sizes. With this commit we ergonomically choose different G1GC options for heap sizes smaller than (not including) 8GB.

Co-authored-by: Daniel Mitterdorfer [email protected]

Our benchmarks have demonstrated that Elasticsearch performs better with the parallel collector than with the default G1 collector when the heap size is small. With this commit we ergonomically choose the parallel collector for heap sizes smaller than (and including) 4GB and turn real memory circuit breaker off when running with ParallelGC. Co-authored-by: Evgenia Badiyanova <[email protected]> Co-authored-by: Daniel Mitterdorfer <[email protected]>

dakrone · 2020-07-15T21:06:18Z

distribution/src/config/jvm.options

@@ -42,7 +42,6 @@
 # following three lines to your version of the JDK


Drive by comment (sorry!), but the comment in jvm.options would be incorrect after this change, since the user would no longer be uncommenting the lines to use G1

hmm, I wonder what we want to add instead.. we want to users to be able to use G1GC before jdk 14, but if they specifically add -XX:+UseG1GC the ergonomics choice won't kick in..

Isn't the point fo the change to make ergonomics choose which GC to use? It doesn't just have to be based on small heaps, it can be based on java version as well?

I updated the jvm.options, any comments/thoughts are welcome!

pugnascotia · 2020-07-23T09:07:30Z

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

        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>[^}]+)}"


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?

rjernst · 2020-07-20T20:27:12Z

server/src/main/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerService.java

-        Setting.boolSetting("indices.breaker.total.use_real_memory", true, Property.NodeScope);
+        Setting.boolSetting("indices.breaker.total.use_real_memory", settings -> {
+            // turn real memory circuit breaker off for ParallelGC
+            return String.valueOf(!JvmInfo.jvmInfo().useParallelGC().equals("true"));


Can we use Booleans.parseBoolean? Also, please use == false for negation.

rjernst · 2020-07-28T17:30:56Z

distribution/src/config/jvm.options

@@ -42,7 +42,6 @@
 # following three lines to your version of the JDK


Isn't the point fo the change to make ergonomics choose which GC to use? It doesn't just have to be based on small heaps, it can be based on java version as well?

…-ergonomics-parallelgc

elasticmachine · 2020-07-31T16:20:44Z

Pinging @elastic/es-core-infra (:Core/Infra/Core)

rjernst · 2020-07-31T17:43:36Z

distribution/src/config/jvm.options

 # 8-13:-XX:-UseConcMarkSweepGC
 # 8-13:-XX:-UseCMSInitiatingOccupancyOnly
-14-:-XX:+UseG1GC
 14-:-XX:G1ReservePercent=25


won't these G1 options break the system if parallelGC is selected?

with -XX:+UseParallelGC java seem to ignore -XX:G1ReservePercent=25 and XX:InitiatingHeapOccupancyPercent them as long as -XX:+UseG1GC is not specified. On the other hand it might be confusing to have G1GC options there when running ParallelGC.

Have you tested if that is true for all G1 options? I wonder if we should completely move all GC options into ergonomics.

henningandersen

I am somewhat concerned about turning off the real memory circuit breaker for small heaps. I wonder if G1 GC tuning could be done instead to bring performance closer to what we get with parallel GC?

henningandersen · 2020-08-04T07:07:57Z

server/src/main/java/org/elasticsearch/indices/breaker/HierarchyCircuitBreakerService.java

-        Setting.boolSetting("indices.breaker.total.use_real_memory", true, Property.NodeScope);
+        Setting.boolSetting("indices.breaker.total.use_real_memory", settings -> {
+            // turn real memory circuit breaker off for ParallelGC
+            return String.valueOf(Booleans.parseBoolean(JvmInfo.jvmInfo().useParallelGC()) == false);


In #46751, a check is made purely against the parent breaker, relying on this being the real memory circuit breaker. This PR disables that PR by default for small heaps.

I seem to remember seeing that approach (just checking the parent breaker, now that we have real memory circuit breaker) has come up a few times.

…-ergonomics-parallelgc

of ParallelGC

…-ergonomics-parallelgc

henningandersen

Thanks @ebadyano , I added a couple of comments to the specific ergonomics chosen.

henningandersen · 2020-10-02T14:39:20Z

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

+    static boolean tuneG1GCForSmallHeap(final Map<String, JvmOption> finalJvmOptions, final long heapSize) {
+        JvmOption g1GC = finalJvmOptions.get("UseG1GC");
+        JvmOption g1GCHeapRegion = finalJvmOptions.get("G1HeapRegionSize");
+        return (heapSize <= 4L << 30 && g1GC.getMandatoryValue().equals("true") && g1GCHeapRegion.isCommandLineOrigin() == false);


I think we should turn the heap size check into heapSize < 8L << 30, since java before version 15 will round the region size down, causing a 7.99GB heap to have a region size of 2MB.

Sorry missread the comment. Make sense, so the tuning options will be for heaps strictly smaller than 8G.

henningandersen · 2020-10-02T15:07:32Z

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

+        final boolean tuneG1GCForSmallHeap = tuneG1GCForSmallHeap(finalJvmOptions, heapSize);
+        final boolean tuneG1GCForLargeHeap = tuneG1GCForLargeHeap(finalJvmOptions, heapSize);
+        if (tuneG1GCForSmallHeap) {
+            ergonomicChoices.add("-XX:G1HeapRegionSize=4m");


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.

rjernst · 2020-10-02T17:37:26Z

distribution/src/config/jvm.options

-14-:-XX:+UseG1GC
-14-:-XX:G1ReservePercent=25
-14-:-XX:InitiatingHeapOccupancyPercent=30
+# 14-:-XX:+UseG1GC


Since we are not choosing the actual GC implementation in ergonomics, doesn't this need to stay uncommented?

Or if we are relying on G1GC being the default now, then do we still need this line in the jvm options at all? Will the ergonomics detect using G1GC when it is not explicitly added to the options?

Thank you for catching this. I should've uncommented the line 14-:-XX:+UseG1GC. I think we still want to keep it uncommented in case someone wants to use G1GC with jdk older than jdk14. Then the comment above still makes sense.

henningandersen

I added a couple of comments, otherwise looking good.

henningandersen · 2020-10-05T19:06:34Z

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

+        if (tuneG1GCForSmallHeap) {
+            ergonomicChoices.add("-XX:G1HeapRegionSize=4m");
+            ergonomicChoices.add("-XX:G1ReservePercent=15");
+            ergonomicChoices.add("-XX:InitiatingHeapOccupancyPercent=45");


I think we should use 30 in this case too.

henningandersen · 2020-10-05T19:15:26Z

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

        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) {


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.

henningandersen · 2020-10-05T19:16:28Z

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

+            ergonomicChoices.add("-XX:G1HeapRegionSize=4m");
+            ergonomicChoices.add("-XX:G1ReservePercent=15");
+            ergonomicChoices.add("-XX:InitiatingHeapOccupancyPercent=45");
+        } else if (tuneG1GCForLargeHeap) {


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.

Thank you for catching this, I updated the pr.

henningandersen

LGTM, thanks for the extra iteration @ebadyano .

One nit: I would like to have a test added too that using ParallelGC (or any other GC than G1) does not add any of the options. No need for an additional round and not mandatory for merging (can do as follow-up too if you prefer).

rjernst

LGTM too

ebadyano · 2020-10-06T19:00:48Z

@henningandersen @rjernst Thank you for the review!

Relates to elastic/elasticsearch#59667

Our benchmarks have demonstrated that Elasticsearch performs better with `-XX:G1HeapRegionSize=4M` and `-XX:G1ReservePercent=15` options for the small heap sizes. With this commit we ergonomically choose different G1GC options for heap sizes smaller than (not including) 8GB. Co-authored-by: Daniel Mitterdorfer [email protected]

Relates to elastic/elasticsearch#59667

Our benchmarks have demonstrated that Elasticsearch performs better with `-XX:G1HeapRegionSize=4M` and `-XX:G1ReservePercent=15` options for the small heap sizes. With this commit we ergonomically choose different G1GC options for heap sizes smaller than (not including) 8GB. Co-authored-by: Daniel Mitterdorfer [email protected]

Relates to elastic/elasticsearch#59667

jpountz · 2020-10-14T14:26:32Z

I added the release highlight label given the great effect on nightly benchmarks.

YohanSciubukgian · 2021-01-19T12:47:55Z

I'm interested of the impact on the memory usage for tiny heap size <=1Gb.
@ebadyano / @jpountz, Do you have any public link to share about benchmarks (before/after this PR) you are talking about ?
Is this PR applies the dynamic rule to ES for JDK8+ ?
Do you have any further recommendations for tiny heap size (<=1Gb) ?

ebadyano added :Core/Infra/Core Core issues without another label >enhancement labels Jul 15, 2020

dakrone reviewed Jul 15, 2020

View reviewed changes

pugnascotia reviewed Jul 23, 2020

View reviewed changes

rjernst reviewed Jul 28, 2020

View reviewed changes

ebadyano added 7 commits July 28, 2020 19:48

Merge branch 'master' of github.com:elastic/elasticsearch into master…

ab6059d

…-ergonomics-parallelgc

Merge branch 'master' of github.com:elastic/elasticsearch into master…

2655315

…-ergonomics-parallelgc

Addressed some comments and fixed tests.

2003331

Fix precomit erorrs

3c05949

Merge branch 'master' of github.com:elastic/elasticsearch into master…

818226a

…-ergonomics-parallelgc

Merge branch 'master' of github.com:elastic/elasticsearch into master…

f14d2cd

…-ergonomics-parallelgc

Fix test

4d033a4

ebadyano marked this pull request as ready for review July 31, 2020 16:20

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jul 31, 2020

rjernst reviewed Jul 31, 2020

View reviewed changes

henningandersen reviewed Aug 4, 2020

View reviewed changes

ebadyano added 3 commits September 22, 2020 18:47

Merge branch 'master' of github.com:elastic/elasticsearch into master…

9008ede

…-ergonomics-parallelgc

Changed the ergonomics heuristic to use G1GC with tuned options instead

d0e34fe

of ParallelGC

Merge branch 'master' of github.com:elastic/elasticsearch into master…

70b4de9

…-ergonomics-parallelgc

ebadyano changed the title ~~Use the parallel collector for small heaps~~ Use different G1GC options for small heaps Sep 29, 2020

ebadyano added 4 commits September 29, 2020 11:17

fix precomit errors

a6f5302

Merge branch 'master' of github.com:elastic/elasticsearch into master…

ffec016

…-ergonomics-parallelgc

Fix precomit errors

80b713b

Fix indentation

411cfd4

ebadyano requested review from henningandersen and rjernst October 2, 2020 14:23

henningandersen reviewed Oct 2, 2020

View reviewed changes

rjernst reviewed Oct 2, 2020

View reviewed changes

henningandersen reviewed Oct 5, 2020

View reviewed changes

ebadyano added 2 commits October 5, 2020 16:16

Address Henning's comments

ac9a568

address Henning's comments

f326f5b

ebadyano requested a review from henningandersen October 5, 2020 20:31

ebadyano added 3 commits October 5, 2020 16:41

Fix tests

5ddb619

Fix precommit tests

d628ff9

Fix logic

d8a3840

henningandersen approved these changes Oct 6, 2020

View reviewed changes

Add an extra test for when non G1GC policy is set

f2e999a

rjernst approved these changes Oct 6, 2020

View reviewed changes

ebadyano merged commit b29423f into elastic:master Oct 6, 2020

ebadyano added a commit to ebadyano/rally-teams that referenced this pull request Oct 7, 2020

Change G1GC options to match elasticsearch jvm.options

7a0797e

Relates to elastic/elasticsearch#59667

ebadyano mentioned this pull request Oct 7, 2020

Change G1GC options to match elasticsearch jvm.options elastic/rally-teams#56

Merged

dliappis added the backport pending label Oct 7, 2020

ebadyano added a commit to elastic/rally-teams that referenced this pull request Oct 8, 2020

Change G1GC options to match elasticsearch jvm.options (#56)

c50cf90

Relates to elastic/elasticsearch#59667

dliappis mentioned this pull request Oct 13, 2020

Elasticsearch didn't start during :example-plugins:script-expert-scoring:yamlRestTest (elasticsearch-master-g1gc) reporting: Multiple garbage collectors selected #63611

Closed

ebadyano added a commit to elastic/rally-teams that referenced this pull request Oct 13, 2020

Change G1GC options to match elasticsearch jvm.options (#56)

ddcdfb9

Relates to elastic/elasticsearch#59667

jpountz added the release highlight label Oct 14, 2020

henningandersen added v7.11.0 v8.0.0 and removed backport pending labels Dec 7, 2020

This was referenced Jan 6, 2021

Clarify G1GC support on docs #43885

Closed

Document how to use the G1 garbage collector #41358

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

		@@ -42,7 +42,6 @@
		# following three lines to your version of the JDK

Use different G1GC options for small heaps #59667

Use different G1GC options for small heaps #59667

Uh oh!

Conversation

ebadyano commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebadyano Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ebadyano Oct 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

ebadyano commented Oct 6, 2020

Uh oh!

jpountz commented Oct 14, 2020

Uh oh!

YohanSciubukgian commented Jan 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ebadyano commented Jul 15, 2020 •

edited

Loading

ebadyano Jul 29, 2020 •

edited

Loading

ebadyano Oct 2, 2020 •

edited

Loading

YohanSciubukgian commented Jan 19, 2021 •

edited

Loading