-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Choose JVM options ergonomically #30684
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
Merged
danielmitterdorfer
merged 3 commits into
elastic:master
from
danielmitterdorfer:ergonomics
Jun 20, 2018
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
108 changes: 108 additions & 0 deletions
108
...bution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/JvmErgonomics.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| /* | ||
| * Licensed to Elasticsearch under one or more contributor | ||
| * license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright | ||
| * ownership. Elasticsearch licenses this file to you under | ||
| * the Apache License, Version 2.0 (the "License"); you may | ||
| * not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.elasticsearch.tools.launchers; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Locale; | ||
| import java.util.Map; | ||
| import java.util.regex.Matcher; | ||
| import java.util.regex.Pattern; | ||
|
|
||
| /** | ||
| * Tunes Elasticsearch JVM settings based on inspection of provided JVM options. | ||
| */ | ||
| final class JvmErgonomics { | ||
| private static final long KB = 1024L; | ||
|
|
||
| private static final long MB = 1024L * 1024L; | ||
|
|
||
| private static final long GB = 1024L * 1024L * 1024L; | ||
|
|
||
|
|
||
| private JvmErgonomics() { | ||
| throw new AssertionError("No instances intended"); | ||
| } | ||
|
|
||
| /** | ||
| * Chooses additional JVM options for Elasticsearch. | ||
| * | ||
| * @param userDefinedJvmOptions A list of JVM options that have been defined by the user. | ||
| * @return A list of additional JVM options to set. | ||
| */ | ||
| static List<String> choose(List<String> userDefinedJvmOptions) { | ||
| List<String> ergonomicChoices = new ArrayList<>(); | ||
| Long heapSize = extractHeapSize(userDefinedJvmOptions); | ||
| Map<String, String> systemProperties = extractSystemProperties(userDefinedJvmOptions); | ||
| if (heapSize != null) { | ||
| if (systemProperties.containsKey("io.netty.allocator.type") == false) { | ||
| if (heapSize <= 1 * GB) { | ||
| ergonomicChoices.add("-Dio.netty.allocator.type=unpooled"); | ||
| } else { | ||
| ergonomicChoices.add("-Dio.netty.allocator.type=pooled"); | ||
| } | ||
| } | ||
| } | ||
| return ergonomicChoices; | ||
| } | ||
|
|
||
| private static final Pattern MAX_HEAP_SIZE = Pattern.compile("^(-Xmx|-XX:MaxHeapSize=)(?<size>\\d+)(?<unit>\\w)?$"); | ||
|
|
||
| // package private for testing | ||
| static Long extractHeapSize(List<String> userDefinedJvmOptions) { | ||
| for (String jvmOption : userDefinedJvmOptions) { | ||
| final Matcher matcher = MAX_HEAP_SIZE.matcher(jvmOption); | ||
| if (matcher.matches()) { | ||
| final long size = Long.parseLong(matcher.group("size")); | ||
| final String unit = matcher.group("unit"); | ||
| if (unit == null) { | ||
| return size; | ||
| } else { | ||
| switch (unit.toLowerCase(Locale.ROOT)) { | ||
| case "k": | ||
| return size * KB; | ||
| case "m": | ||
| return size * MB; | ||
| case "g": | ||
| return size * GB; | ||
| default: | ||
| throw new IllegalArgumentException("Unknown unit [" + unit + "] for max heap size in [" + jvmOption + "]"); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private static final Pattern SYSTEM_PROPERTY = Pattern.compile("^-D(?<key>[\\w+].*?)=(?<value>.*)$"); | ||
|
|
||
| // package private for testing | ||
| static Map<String, String> extractSystemProperties(List<String> userDefinedJvmOptions) { | ||
| Map<String, String> systemProperties = new HashMap<>(); | ||
| for (String jvmOption : userDefinedJvmOptions) { | ||
| final Matcher matcher = SYSTEM_PROPERTY.matcher(jvmOption); | ||
| if (matcher.matches()) { | ||
| systemProperties.put(matcher.group("key"), matcher.group("value")); | ||
| } | ||
| } | ||
| return systemProperties; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
83 changes: 83 additions & 0 deletions
83
...n/tools/launchers/src/test/java/org/elasticsearch/tools/launchers/JvmErgonomicsTests.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| /* | ||
| * Licensed to Elasticsearch under one or more contributor | ||
| * license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright | ||
| * ownership. Elasticsearch licenses this file to you under | ||
| * the Apache License, Version 2.0 (the "License"); you may | ||
| * not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.elasticsearch.tools.launchers; | ||
|
|
||
| import java.util.Arrays; | ||
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertNull; | ||
| import static org.junit.Assert.assertTrue; | ||
| import static org.junit.Assert.fail; | ||
|
|
||
| public class JvmErgonomicsTests extends LaunchersTestCase { | ||
| public void testExtractValidHeapSize() { | ||
| assertEquals(Long.valueOf(1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx1024"))); | ||
| assertEquals(Long.valueOf(2L * 1024 * 1024 * 1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx2g"))); | ||
| assertEquals(Long.valueOf(32 * 1024 * 1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx32M"))); | ||
| assertEquals(Long.valueOf(32 * 1024 * 1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-XX:MaxHeapSize=32M"))); | ||
| } | ||
|
|
||
| public void testExtractInvalidHeapSize() { | ||
| try { | ||
| JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx2T")); | ||
| fail("Expected IllegalArgumentException to be raised"); | ||
| } catch (IllegalArgumentException expected) { | ||
| assertEquals("Unknown unit [T] for max heap size in [-Xmx2T]", expected.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| public void testExtractNoHeapSize() { | ||
| assertNull("No spaces allowed", JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx 1024"))); | ||
| assertNull("JVM option is not present", JvmErgonomics.extractHeapSize(Collections.singletonList(""))); | ||
| assertNull("Multiple JVM options per line", JvmErgonomics.extractHeapSize(Collections.singletonList("-Xms2g -Xmx2g"))); | ||
| } | ||
|
|
||
| public void testExtractSystemProperties() { | ||
| Map<String, String> expectedSystemProperties = new HashMap<>(); | ||
| expectedSystemProperties.put("file.encoding", "UTF-8"); | ||
| expectedSystemProperties.put("kv.setting", "ABC=DEF"); | ||
|
|
||
| Map<String, String> parsedSystemProperties = JvmErgonomics.extractSystemProperties( | ||
| Arrays.asList("-Dfile.encoding=UTF-8", "-Dkv.setting=ABC=DEF")); | ||
|
|
||
| assertEquals(expectedSystemProperties, parsedSystemProperties); | ||
| } | ||
|
|
||
| public void testExtractNoSystemProperties() { | ||
| Map<String, String> parsedSystemProperties = JvmErgonomics.extractSystemProperties(Arrays.asList("-Xms1024M", "-Xmx1024M")); | ||
| assertTrue(parsedSystemProperties.isEmpty()); | ||
| } | ||
|
|
||
| public void testLittleMemoryErgonomicChoices() { | ||
| String smallHeap = randomFrom(Arrays.asList("64M", "512M", "1024M", "1G")); | ||
| List<String> expectedChoices = Collections.singletonList("-Dio.netty.allocator.type=unpooled"); | ||
| assertEquals(expectedChoices, JvmErgonomics.choose(Arrays.asList("-Xms" + smallHeap, "-Xmx" + smallHeap))); | ||
| } | ||
|
|
||
| public void testPlentyMemoryErgonomicChoices() { | ||
| String largeHeap = randomFrom(Arrays.asList("1025M", "2048M", "2G", "8G")); | ||
| List<String> expectedChoices = Collections.singletonList("-Dio.netty.allocator.type=pooled"); | ||
| assertEquals(expectedChoices, JvmErgonomics.choose(Arrays.asList("-Xms" + largeHeap, "-Xmx" + largeHeap))); | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Would it be possible to return what the default JVM heap is here instead of null? Seems like we still may want to make changes to JVM options for an unset heap.
Either that, or since we explicitly set it in our
jvm.options, emit a warning or error that no heap has been specified (someone must have removed the option)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.
The JVM chooses the heap size ergonomically when the heap size is not specified.
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.
Do we have the ability to see what it would choose? If running on a small machine, for instance, we'd still want to disable Netty's pooled allocator if the JVM is going to automatically choose a 400mb heap
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 can be quite tricky because the ergonomics these days depend on other flags on the JVM (e.g.,
-XX:+UseCGroupMemoryLimitForHeap).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.
We could try starting a JVM with all the flags specified and
-XX:+PrintFlagsFinal -versionand scrape the output forMaxHeapSize.Uh oh!
There was an error while loading. Please reload this page.
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.
One aspect that I like about that approach is that it is pretty lightweight (it just checks the provided arguments). Yes, it will not choose settings ergonomically if the user has manually removed the heap size setting from
jvm.options. However, I think this is (a) not the common case as this is probably one of the most important JVM-related settings in Elasticsearch and (b) we'd lose the ability to provide an ergonomic choice but otherwise everything works as is.Jason has mentioned a good approach to get around this limitation. I think it would work but I see the following potential risks:
javaprocess with the very same settings as Elasticsearch so this will negatively affect our startup time to a small degree (I rantime java -Xms8G -Xmx8G -XX:+UnlockDiagnosticVMOptions -XX:+AlwaysPreTouch -XX:+PrintFlagsFinal -Xlog:gc\*,gc+age=trace,safepoint:file=/tmp/gc.log:utctime,pid,tags:filecount=32,filesize=64m -versionon several machines and measured between 0.5 and 3 seconds Wall clock time). We'd also output several GC log files (every JVM startup creates a new one) which might be confusing.-XXflag (-XX:+PrintFlagsFinal) to parse output. In theory-XXmay disappear without any prior notice. Personally, I doubt though that this would happen with-XX:+PrintFlagsFinal.In the end both approaches have their advantages and disadvantages but if we contrast this with the requirement that the user specifies
-Xmxin order for us to provide ergonomic choices I think the current approach is a reasonable compromise? I think though that it would be nice to emit a warning that ergonomic choices are turned off in case we cannot detect a heap size?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.
@danielmitterdorfer The
-versionavoids actually starting a JVM so we can-XX:+AlwaysPreTouchwith no impact.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.
Thanks for the correction. I've corrected my comment accordingly.
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 this is a reasonable solution, right now since we ship a jvm.options it would require a user to go in and remove the settings that we have, so I don't think it will be a common case.
We don't remove the options and use
-XX:+UseCGroupMemoryLimitForHeapfor our docker image(s) do we?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.
We do not do that.