Warn when MaxDirectMemorySize may be incorrect (Windows/JDK8 only issue) #48365
Conversation
This file contains 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
Our JVM ergonomics extract max heap size from JDK PrintFlagsFinal output. On JDK 8, there is a system-dependent bug where memory sizes are cast to 32-bit integers. On affected systems (namely, Windows), when 1/4 of physical memory is more than the maximum integer value, the output of PrintFlagsFinal will be inaccurate. In the pathological case, where the max heap size would be a multiple of 4g, the test will fail. This commit mutes the test for that specific circumstance, but I will need to do some further investigation of the consequences of this behavior.
williamrandolph
added
>bug
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (:Core/Infra/Core) |
|
@elasticmachine run elasticsearch-ci/1 |
|
@elasticmachine run elasticsearch-ci/bwc |
alpar-t
reviewed
Oct 23, 2019
...tion/tools/launchers/src/test/java/org/elasticsearch/tools/launchers/JvmErgonomicsTests.java
Show resolved
Hide resolved
alpar-t
reviewed
Oct 23, 2019
distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/JvmErgonomics.java
Show resolved
Hide resolved
|
Thanks for investigating this issue! A warning seems fine to me, and the code here looks good, but I'd like @danielmitterdorfer to weigh in since he has been the primary implementor of the jvm ergonomics. |
danielmitterdorfer
approved these changes
Oct 24, 2019
There was a problem hiding this comment.
Thanks for spotting this! Looks fine to me. I have just one small suggestion. We could link to the OpenJDK bug tracker instead (https://bugs.openjdk.java.net/browse/JDK-8074459) or don't include the link at all (I think we've tried to avoid this so far) and only reference the bug number JDK-8074459.
|
@elasticmachine run elasticsearch-ci/default-distro |
williamrandolph
added a commit
to williamrandolph/elasticsearch
that referenced
this pull request
Oct 25, 2019
…ue) (elastic#48365) Our JVM ergonomics extract max heap size from JDK PrintFlagsFinal output. On JDK 8, there is a system-dependent bug where memory sizes are cast to 32-bit integers. On affected systems (namely, Windows), when 1/4 of physical memory is more than the maximum integer value, the output of PrintFlagsFinal will be inaccurate. In the pathological case, where the max heap size would be a multiple of 4g, the test will fail. The practical effect of this bug, beyond test failures, is that we may set MaxDirectMemorySize to an incorrect value on Windows. This commit adds a warning about this situation during startup.
This was referenced Oct 25, 2019
williamrandolph
added a commit
to williamrandolph/elasticsearch
that referenced
this pull request
Nov 11, 2019
…ue) (elastic#48365) Our JVM ergonomics extract max heap size from JDK PrintFlagsFinal output. On JDK 8, there is a system-dependent bug where memory sizes are cast to 32-bit integers. On affected systems (namely, Windows), when 1/4 of physical memory is more than the maximum integer value, the output of PrintFlagsFinal will be inaccurate. In the pathological case, where the max heap size would be a multiple of 4g, the test will fail. The practical effect of this bug, beyond test failures, is that we may set MaxDirectMemorySize to an incorrect value on Windows. This commit adds a warning about this situation during startup.
williamrandolph
added a commit
to williamrandolph/elasticsearch
that referenced
this pull request
Nov 12, 2019
…ue) (elastic#48365) Our JVM ergonomics extract max heap size from JDK PrintFlagsFinal output. On JDK 8, there is a system-dependent bug where memory sizes are cast to 32-bit integers. On affected systems (namely, Windows), when 1/4 of physical memory is more than the maximum integer value, the output of PrintFlagsFinal will be inaccurate. In the pathological case, where the max heap size would be a multiple of 4g, the test will fail. The practical effect of this bug, beyond test failures, is that we may set MaxDirectMemorySize to an incorrect value on Windows. This commit adds a warning about this situation during startup. On 7.4, we also warn for io.netty.allocator.type.
williamrandolph
added a commit
that referenced
this pull request
Nov 12, 2019
* Warn when MaxDirectMemorySize may be incorrect (Windows/JDK8 only issue) (#48365) Our JVM ergonomics extract max heap size from JDK PrintFlagsFinal output. On JDK 8, there is a system-dependent bug where memory sizes are cast to 32-bit integers. On affected systems (namely, Windows), when 1/4 of physical memory is more than the maximum integer value, the output of PrintFlagsFinal will be inaccurate. In the pathological case, where the max heap size would be a multiple of 4g, the test will fail. The practical effect of this bug, beyond test failures, is that we may set MaxDirectMemorySize to an incorrect value on Windows. This commit adds a warning about this situation during startup. * Don't drop user's MaxDirectMemorySize flag on jdk8/windows (#48657) * Always pass user-specified MaxDirectMemorySize We had been testing whether a user had passed a value for MaxDirectMemorySize by parsing the output of "java -XX:PrintFlagsFinal -version". If MaxDirectMemorySize equals zero, we set it to half of max heap. The problem is that on Windows with JDK 8, a JDK bug incorrectly truncates values over 4g and returns multiples of 4g as zero. In order to always respect the user-defined settings, we need to check our input to see if an "-XX:MaxDirectMemorySize" value has been passed. * Always warn for Windows/jdk8 ergo issue Even if a user has set MaxDirectMemorySize, they aren't future-proof for this JDK bug. With this change, we issue a general warning for the windows/JDK8 issue, and a specific warning if MaxDirectMemorySize is unset.
williamrandolph
added a commit
that referenced
this pull request
Nov 15, 2019
…8/Windows (#49043) * Warn when MaxDirectMemorySize may be incorrect (Windows/JDK8 only issue) (#48365) Our JVM ergonomics extract max heap size from JDK PrintFlagsFinal output. On JDK 8, there is a system-dependent bug where memory sizes are cast to 32-bit integers. On affected systems (namely, Windows), when 1/4 of physical memory is more than the maximum integer value, the output of PrintFlagsFinal will be inaccurate. In the pathological case, where the max heap size would be a multiple of 4g, the test will fail. The practical effect of this bug, beyond test failures, is that we may set MaxDirectMemorySize to an incorrect value on Windows. This commit adds a warning about this situation during startup. On 7.4, we also warn for io.netty.allocator.type. * Don't drop user's MaxDirectMemorySize flag on jdk8/windows (#48657) * Always pass user-specified MaxDirectMemorySize We had been testing whether a user had passed a value for MaxDirectMemorySize by parsing the output of "java -XX:PrintFlagsFinal -version". If MaxDirectMemorySize equals zero, we set it to half of max heap. The problem is that on Windows with JDK 8, a JDK bug incorrectly truncates values over 4g and returns multiples of 4g as zero. In order to always respect the user-defined settings, we need to check our input to see if an "-XX:MaxDirectMemorySize" value has been passed. * Always warn for Windows/jdk8 ergo issue Even if a user has set MaxDirectMemorySize, they aren't future-proof for this JDK bug. With this change, we issue a general warning for the windows/JDK8 issue, and a specific warning if MaxDirectMemorySize is unset. * Adjust warning for io.netty.allocator.type
This was referenced Feb 3, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 is a bug in JDK 8 for Windows where, with flags such as
-XX:+PrintFlagsFinal, memory sizes are printed as unsigned 32-bit integers. The maximum value of such an integer is2^32 - 1, which is one byte less than the value of4gin a JVM argument. If you pass-XX:+PrintFlagsFinal -Xmx=4g, you will see a heap size of zero. This bug only seems to affect display; the JVM still starts with the correct heap size. On Windows and Linux, the size value is cast to an unsigned 64-bit integer, which has plenty of room for holding realistic memory sizes. In JDK 9 and beyond, this bug was fixed, so any memory size will be displayed correctly.This issue affects our JvmErgonomics code. In order to set MaxDirectMemorySize, we parse the output from
java -XX:+PrintFlagsFinal -version(adding whichever flags we'd like to see), retrieve the value for MaxHeapSize, and set MaxDirectMemorySize to half of heap. However, if the user has provided a value for MaxDirectMemorySize, we use the provided value.Consider a system with 40G of physical RAM. By default, the JVM would set the MaxHeapSize at one quarter of that, which is to say, 10G. Our intention would be to set MaxDirectMemorySize to 5G. But on Windows, JDK 8 will print MaxHeapSize as 2G, and MaxDirectMemorySize will be set to 1G.
The workaround is simple: set
-XX:MaxDirectMemorySizedirectly.If we wanted to fix this problem under the hood, we'd have to do two things when running on Windows with JDK8:
OperatingSystemMXBeanto find the system's physical memory. Use that to set a correct heap size and a correct MaxDirectMemorySize.Not only does the above seem like brittle logic, it also puts us in the business of duplicating Java's logic for parsing memory values. The virtue of our current implementation is that we don't have to do that at all.
Note that starting with 8.0.0, we will require a Java 11 runtime, so this problem won't happen on the 8.x line.
In light of that level of complexity, I think it's sufficient to emit a warning when running on Windows with Java 8 when there's no explicit setting for MaxDirectMemorySize. I am not sure how bad it is to have an incorrect setting (a setting of 0 in the worst case), but I could easily be persuaded that we should fail to start without an explicit MaxDirectMemorySize setting in a case where our JVM Ergonomics would set it incorrectly, but that would be a breaking change to startup configuration for some users and we'd probably have to flag it in release notes or something.
Fixes #47384