Skip to content

Commit 2592b49

Browse files
authored
Remove manual parsing of JVM options (#41962)
This commit removes manual parsing of JVM options when calculating ergonomics. This is to avoid a situation that we parse values differently than the JVM would. In fact, we already have a bug along these lines today. It is possible to start the JVM with the same flag multiple times on the command line. In this case, the last value wins. For example, -Xmx1g -Xmx2g would start the JVM with a heap size of two gigabytes. Our JVM ergonomics ignores this possibility and instead the first value is winning! Our strategy to avoid manual parsing of the JVM options is to start the Java command line parser (without actually starting a JVM) by invoking java with the same command line flags as presented and request that the JVM tell us what values it would start with. This ensures that we have the correct values when making ergonomic decisions. Moreover, our strategy also is ignoring ES_JAVA_OPTS which could override the heap size as well leading to incorrect ergonomic choices. This commit address this issue too.
1 parent f2a558d commit 2592b49

File tree

6 files changed

+138
-67
lines changed

6 files changed

+138
-67
lines changed

distribution/src/bin/elasticsearch

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ source "`dirname "$0"`"/elasticsearch-env
1818

1919
ES_JVM_OPTIONS="$ES_PATH_CONF"/jvm.options
2020
JVM_OPTIONS=`"$JAVA" -cp "$ES_CLASSPATH" org.elasticsearch.tools.launchers.JvmOptionsParser "$ES_JVM_OPTIONS"`
21-
ES_JAVA_OPTS="${JVM_OPTIONS//\$\{ES_TMPDIR\}/$ES_TMPDIR} $ES_JAVA_OPTS"
21+
ES_JAVA_OPTS="${JVM_OPTIONS//\$\{ES_TMPDIR\}/$ES_TMPDIR}"
2222

2323
# manual parsing to find out, if process should be detached
2424
if ! echo $* | grep -E '(^-d |-d$| -d |--daemonize$|--daemonize )' > /dev/null; then

distribution/src/bin/elasticsearch-service.bat

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,7 @@ if not "%ES_JAVA_OPTS%" == "" set ES_JAVA_OPTS=%ES_JAVA_OPTS: =;%
112112

113113
@setlocal
114114
for /F "usebackq delims=" %%a in (`"%JAVA% -cp "!ES_CLASSPATH!" "org.elasticsearch.tools.launchers.JvmOptionsParser" "!ES_JVM_OPTIONS!" || echo jvm_options_parser_failed"`) do set JVM_OPTIONS=%%a
115-
@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%JVM_OPTIONS%" & set ES_JAVA_OPTS=%JVM_OPTIONS:${ES_TMPDIR}=!ES_TMPDIR!% %ES_JAVA_OPTS%
115+
@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%JVM_OPTIONS%" & set ES_JAVA_OPTS=%JVM_OPTIONS:${ES_TMPDIR}=!ES_TMPDIR!%
116116

117117
if "%MAYBE_JVM_OPTIONS_PARSER_FAILED%" == "jvm_options_parser_failed" (
118118
exit /b 1

distribution/src/bin/elasticsearch.bat

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ IF ERRORLEVEL 1 (
4444
set ES_JVM_OPTIONS=%ES_PATH_CONF%\jvm.options
4545
@setlocal
4646
for /F "usebackq delims=" %%a in (`CALL %JAVA% -cp "!ES_CLASSPATH!" "org.elasticsearch.tools.launchers.JvmOptionsParser" "!ES_JVM_OPTIONS!" ^|^| echo jvm_options_parser_failed`) do set JVM_OPTIONS=%%a
47-
@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%JVM_OPTIONS%" & set ES_JAVA_OPTS=%JVM_OPTIONS:${ES_TMPDIR}=!ES_TMPDIR!% %ES_JAVA_OPTS%
47+
@endlocal & set "MAYBE_JVM_OPTIONS_PARSER_FAILED=%JVM_OPTIONS%" & set ES_JAVA_OPTS=%JVM_OPTIONS:${ES_TMPDIR}=!ES_TMPDIR!%
4848

4949
if "%MAYBE_JVM_OPTIONS_PARSER_FAILED%" == "jvm_options_parser_failed" (
5050
exit /b 1

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

Lines changed: 70 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -19,24 +19,27 @@
1919

2020
package org.elasticsearch.tools.launchers;
2121

22+
import java.io.BufferedReader;
23+
import java.io.IOException;
24+
import java.io.InputStream;
25+
import java.io.InputStreamReader;
26+
import java.nio.charset.StandardCharsets;
27+
import java.nio.file.Path;
2228
import java.util.ArrayList;
2329
import java.util.HashMap;
2430
import java.util.List;
2531
import java.util.Locale;
2632
import java.util.Map;
33+
import java.util.Optional;
2734
import java.util.regex.Matcher;
2835
import java.util.regex.Pattern;
36+
import java.util.stream.Collectors;
37+
import java.util.stream.Stream;
2938

3039
/**
3140
* Tunes Elasticsearch JVM settings based on inspection of provided JVM options.
3241
*/
3342
final class JvmErgonomics {
34-
private static final long KB = 1024L;
35-
36-
private static final long MB = 1024L * 1024L;
37-
38-
private static final long GB = 1024L * 1024L * 1024L;
39-
4043

4144
private JvmErgonomics() {
4245
throw new AssertionError("No instances intended");
@@ -48,48 +51,73 @@ private JvmErgonomics() {
4851
* @param userDefinedJvmOptions A list of JVM options that have been defined by the user.
4952
* @return A list of additional JVM options to set.
5053
*/
51-
static List<String> choose(List<String> userDefinedJvmOptions) {
52-
List<String> ergonomicChoices = new ArrayList<>();
53-
Long heapSize = extractHeapSize(userDefinedJvmOptions);
54-
Map<String, String> systemProperties = extractSystemProperties(userDefinedJvmOptions);
55-
if (heapSize != null) {
56-
if (systemProperties.containsKey("io.netty.allocator.type") == false) {
57-
if (heapSize <= 1 * GB) {
58-
ergonomicChoices.add("-Dio.netty.allocator.type=unpooled");
59-
} else {
60-
ergonomicChoices.add("-Dio.netty.allocator.type=pooled");
61-
}
54+
static List<String> choose(final List<String> userDefinedJvmOptions) throws InterruptedException, IOException {
55+
final List<String> ergonomicChoices = new ArrayList<>();
56+
final Map<String, Optional<String>> finalJvmOptions = finalJvmOptions(userDefinedJvmOptions);
57+
final long heapSize = extractHeapSize(finalJvmOptions);
58+
final Map<String, String> systemProperties = extractSystemProperties(userDefinedJvmOptions);
59+
if (systemProperties.containsKey("io.netty.allocator.type") == false) {
60+
if (heapSize <= 1 << 30) {
61+
ergonomicChoices.add("-Dio.netty.allocator.type=unpooled");
62+
} else {
63+
ergonomicChoices.add("-Dio.netty.allocator.type=pooled");
6264
}
6365
}
6466
return ergonomicChoices;
6567
}
6668

67-
private static final Pattern MAX_HEAP_SIZE = Pattern.compile("^(-Xmx|-XX:MaxHeapSize=)(?<size>\\d+)(?<unit>\\w)?$");
69+
private static final Pattern OPTION =
70+
Pattern.compile("^\\s*\\S+\\s+(?<flag>\\S+)\\s+:?=\\s+(?<value>\\S+)?\\s+\\{[^}]+?\\}\\s+\\{[^}]+}");
6871

69-
// package private for testing
70-
static Long extractHeapSize(List<String> userDefinedJvmOptions) {
71-
for (String jvmOption : userDefinedJvmOptions) {
72-
final Matcher matcher = MAX_HEAP_SIZE.matcher(jvmOption);
73-
if (matcher.matches()) {
74-
final long size = Long.parseLong(matcher.group("size"));
75-
final String unit = matcher.group("unit");
76-
if (unit == null) {
77-
return size;
78-
} else {
79-
switch (unit.toLowerCase(Locale.ROOT)) {
80-
case "k":
81-
return size * KB;
82-
case "m":
83-
return size * MB;
84-
case "g":
85-
return size * GB;
86-
default:
87-
throw new IllegalArgumentException("Unknown unit [" + unit + "] for max heap size in [" + jvmOption + "]");
88-
}
89-
}
90-
}
72+
static Map<String, Optional<String>> finalJvmOptions(
73+
final List<String> userDefinedJvmOptions) throws InterruptedException, IOException {
74+
return flagsFinal(userDefinedJvmOptions).stream()
75+
.map(OPTION::matcher).filter(Matcher::matches)
76+
.collect(Collectors.toUnmodifiableMap(m -> m.group("flag"), m -> Optional.ofNullable(m.group("value"))));
77+
}
78+
79+
private static List<String> flagsFinal(final List<String> userDefinedJvmOptions) throws InterruptedException, IOException {
80+
/*
81+
* To deduce the final set of JVM options that Elasticsearch is going to start with, we start a separate Java process with the JVM
82+
* options that we would pass on the command line. For this Java process we will add two additional flags, -XX:+PrintFlagsFinal and
83+
* -version. This causes the Java process that we start to parse the JVM options into their final values, display them on standard
84+
* output, print the version to standard error, and then exit. The JVM itself never bootstraps, and therefore this process is
85+
* lightweight. By doing this, we get the JVM options parsed exactly as the JVM that we are going to execute would parse them
86+
* without having to implement our own JVM option parsing logic.
87+
*/
88+
final String java = Path.of(System.getProperty("java.home"), "bin", "java").toString();
89+
final List<String> command =
90+
Stream.of(Stream.of(java), userDefinedJvmOptions.stream(), Stream.of("-XX:+PrintFlagsFinal"), Stream.of("-version"))
91+
.reduce(Stream::concat)
92+
.get()
93+
.collect(Collectors.toUnmodifiableList());
94+
final Process process = new ProcessBuilder().command(command).start();
95+
final List<String> output = readLinesFromInputStream(process.getInputStream());
96+
final List<String> error = readLinesFromInputStream(process.getErrorStream());
97+
final int status = process.waitFor();
98+
if (status != 0) {
99+
final String message = String.format(
100+
Locale.ROOT,
101+
"starting java failed with [%d]\noutput:\n%s\nerror:\n%s",
102+
status,
103+
String.join("\n", output),
104+
String.join("\n", error));
105+
throw new RuntimeException(message);
106+
} else {
107+
return output;
108+
}
109+
}
110+
111+
private static List<String> readLinesFromInputStream(final InputStream is) throws IOException {
112+
try (InputStreamReader isr = new InputStreamReader(is, StandardCharsets.UTF_8);
113+
BufferedReader br = new BufferedReader(isr)) {
114+
return br.lines().collect(Collectors.toUnmodifiableList());
91115
}
92-
return null;
116+
}
117+
118+
// package private for testing
119+
static Long extractHeapSize(final Map<String, Optional<String>> finalJvmOptions) {
120+
return Long.parseLong(finalJvmOptions.get("MaxHeapSize").get());
93121
}
94122

95123
private static final Pattern SYSTEM_PROPERTY = Pattern.compile("^-D(?<key>[\\w+].*?)=(?<value>.*)$");
@@ -105,4 +133,5 @@ static Map<String, String> extractSystemProperties(List<String> userDefinedJvmOp
105133
}
106134
return systemProperties;
107135
}
136+
108137
}

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

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,14 @@
1919

2020
package org.elasticsearch.tools.launchers;
2121

22+
import org.elasticsearch.tools.java_version_checker.JavaVersion;
23+
2224
import java.io.BufferedReader;
2325
import java.io.IOException;
2426
import java.io.InputStream;
2527
import java.io.InputStreamReader;
2628
import java.io.Reader;
27-
import java.nio.charset.Charset;
29+
import java.nio.charset.StandardCharsets;
2830
import java.nio.file.Files;
2931
import java.nio.file.Paths;
3032
import java.util.ArrayList;
@@ -35,10 +37,10 @@
3537
import java.util.Map;
3638
import java.util.SortedMap;
3739
import java.util.TreeMap;
40+
import java.util.function.Predicate;
3841
import java.util.regex.Matcher;
3942
import java.util.regex.Pattern;
40-
41-
import org.elasticsearch.tools.java_version_checker.JavaVersion;
43+
import java.util.stream.Collectors;
4244

4345
/**
4446
* Parses JVM options from a file and prints a single line with all JVM options to standard output.
@@ -51,14 +53,14 @@ final class JvmOptionsParser {
5153
*
5254
* @param args the args to the program which should consist of a single option, the path to the JVM options
5355
*/
54-
public static void main(final String[] args) throws IOException {
56+
public static void main(final String[] args) throws InterruptedException, IOException {
5557
if (args.length != 1) {
5658
throw new IllegalArgumentException("expected one argument specifying path to jvm.options but was " + Arrays.toString(args));
5759
}
5860
final List<String> jvmOptions = new ArrayList<>();
5961
final SortedMap<Integer, String> invalidLines = new TreeMap<>();
6062
try (InputStream is = Files.newInputStream(Paths.get(args[0]));
61-
Reader reader = new InputStreamReader(is, Charset.forName("UTF-8"));
63+
Reader reader = new InputStreamReader(is, StandardCharsets.UTF_8);
6264
BufferedReader br = new BufferedReader(reader)) {
6365
parse(
6466
JavaVersion.majorVersion(JavaVersion.CURRENT),
@@ -78,7 +80,14 @@ public void accept(final int lineNumber, final String line) {
7880
}
7981

8082
if (invalidLines.isEmpty()) {
81-
List<String> ergonomicJvmOptions = JvmErgonomics.choose(jvmOptions);
83+
// now append the JVM options from ES_JAVA_OPTS
84+
final String environmentJvmOptions = System.getenv("ES_JAVA_OPTS");
85+
if (environmentJvmOptions != null) {
86+
jvmOptions.addAll(Arrays.stream(environmentJvmOptions.split("\\s+"))
87+
.filter(Predicate.not(String::isBlank))
88+
.collect(Collectors.toUnmodifiableList()));
89+
}
90+
final List<String> ergonomicJvmOptions = JvmErgonomics.choose(jvmOptions);
8291
jvmOptions.addAll(ergonomicJvmOptions);
8392
final String spaceDelimitedJvmOptions = spaceDelimitJvmOptions(jvmOptions);
8493
Launchers.outPrintln(spaceDelimitedJvmOptions);

distribution/tools/launchers/src/test/java/org/elasticsearch/tools/launchers/JvmErgonomicsTests.java

Lines changed: 50 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -19,38 +19,70 @@
1919

2020
package org.elasticsearch.tools.launchers;
2121

22+
import java.io.IOException;
2223
import java.util.Arrays;
2324
import java.util.Collections;
2425
import java.util.HashMap;
2526
import java.util.List;
2627
import java.util.Map;
2728

29+
import static org.hamcrest.Matchers.containsString;
30+
import static org.hamcrest.Matchers.equalTo;
31+
import static org.hamcrest.Matchers.greaterThan;
32+
import static org.hamcrest.Matchers.hasToString;
2833
import static org.junit.Assert.assertEquals;
29-
import static org.junit.Assert.assertNull;
34+
import static org.junit.Assert.assertThat;
3035
import static org.junit.Assert.assertTrue;
3136
import static org.junit.Assert.fail;
3237

3338
public class JvmErgonomicsTests extends LaunchersTestCase {
34-
public void testExtractValidHeapSize() {
35-
assertEquals(Long.valueOf(1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx1024")));
36-
assertEquals(Long.valueOf(2L * 1024 * 1024 * 1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx2g")));
37-
assertEquals(Long.valueOf(32 * 1024 * 1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx32M")));
38-
assertEquals(Long.valueOf(32 * 1024 * 1024), JvmErgonomics.extractHeapSize(Collections.singletonList("-XX:MaxHeapSize=32M")));
39+
40+
public void testExtractValidHeapSizeUsingXmx() throws InterruptedException, IOException {
41+
assertThat(
42+
JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-Xmx2g"))),
43+
equalTo(2L << 30));
44+
}
45+
46+
public void testExtractValidHeapSizeUsingMaxHeapSize() throws InterruptedException, IOException {
47+
assertThat(
48+
JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-XX:MaxHeapSize=2g"))),
49+
equalTo(2L << 30));
50+
}
51+
52+
public void testExtractValidHeapSizeNoOptionPresent() throws InterruptedException, IOException {
53+
assertThat(
54+
JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.emptyList())),
55+
greaterThan(0L));
56+
}
57+
58+
public void testHeapSizeInvalid() throws InterruptedException, IOException {
59+
try {
60+
JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-Xmx2Z")));
61+
fail("expected starting java to fail");
62+
} catch (final RuntimeException e) {
63+
assertThat(e, hasToString(containsString(("starting java failed"))));
64+
assertThat(e, hasToString(containsString(("Invalid maximum heap size: -Xmx2Z"))));
65+
}
3966
}
4067

41-
public void testExtractInvalidHeapSize() {
68+
public void testHeapSizeTooSmall() throws InterruptedException, IOException {
4269
try {
43-
JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx2T"));
44-
fail("Expected IllegalArgumentException to be raised");
45-
} catch (IllegalArgumentException expected) {
46-
assertEquals("Unknown unit [T] for max heap size in [-Xmx2T]", expected.getMessage());
70+
JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-Xmx1024")));
71+
fail("expected starting java to fail");
72+
} catch (final RuntimeException e) {
73+
assertThat(e, hasToString(containsString(("starting java failed"))));
74+
assertThat(e, hasToString(containsString(("Too small maximum heap"))));
4775
}
4876
}
4977

50-
public void testExtractNoHeapSize() {
51-
assertNull("No spaces allowed", JvmErgonomics.extractHeapSize(Collections.singletonList("-Xmx 1024")));
52-
assertNull("JVM option is not present", JvmErgonomics.extractHeapSize(Collections.singletonList("")));
53-
assertNull("Multiple JVM options per line", JvmErgonomics.extractHeapSize(Collections.singletonList("-Xms2g -Xmx2g")));
78+
public void testHeapSizeWithSpace() throws InterruptedException, IOException {
79+
try {
80+
JvmErgonomics.extractHeapSize(JvmErgonomics.finalJvmOptions(Collections.singletonList("-Xmx 1024")));
81+
fail("expected starting java to fail");
82+
} catch (final RuntimeException e) {
83+
assertThat(e, hasToString(containsString(("starting java failed"))));
84+
assertThat(e, hasToString(containsString(("Invalid maximum heap size: -Xmx 1024"))));
85+
}
5486
}
5587

5688
public void testExtractSystemProperties() {
@@ -69,15 +101,16 @@ public void testExtractNoSystemProperties() {
69101
assertTrue(parsedSystemProperties.isEmpty());
70102
}
71103

72-
public void testLittleMemoryErgonomicChoices() {
104+
public void testLittleMemoryErgonomicChoices() throws InterruptedException, IOException {
73105
String smallHeap = randomFrom(Arrays.asList("64M", "512M", "1024M", "1G"));
74106
List<String> expectedChoices = Collections.singletonList("-Dio.netty.allocator.type=unpooled");
75107
assertEquals(expectedChoices, JvmErgonomics.choose(Arrays.asList("-Xms" + smallHeap, "-Xmx" + smallHeap)));
76108
}
77109

78-
public void testPlentyMemoryErgonomicChoices() {
110+
public void testPlentyMemoryErgonomicChoices() throws InterruptedException, IOException {
79111
String largeHeap = randomFrom(Arrays.asList("1025M", "2048M", "2G", "8G"));
80112
List<String> expectedChoices = Collections.singletonList("-Dio.netty.allocator.type=pooled");
81113
assertEquals(expectedChoices, JvmErgonomics.choose(Arrays.asList("-Xms" + largeHeap, "-Xmx" + largeHeap)));
82114
}
115+
83116
}

0 commit comments

Comments
 (0)