Skip to content
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

Warn when MaxDirectMemorySize may be incorrect (Windows/JDK8 only issue) #48365

Conversation

williamrandolph
Copy link
Contributor

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 is 2^32 - 1, which is one byte less than the value of 4g in 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:MaxDirectMemorySize directly.

If we wanted to fix this problem under the hood, we'd have to do two things when running on Windows with JDK8:

  1. Check whether the user has passed in one of the maximum heap size flags (not just -Xmx but also -XX:MaxHeapSize). If so, parse the value for that flag and see if it's over 4g. If so, use the parsed value to set MaxDirectMemorySize.
  2. If MaxHeapSize is unspecified, use an OperatingSystemMXBean to 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

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.
@elasticmachine
Copy link
Collaborator

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

@williamrandolph
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1
@elasticmachine run elasticsearch-ci/bwc

@alpar-t
Copy link
Contributor

alpar-t commented Oct 23, 2019

@elasticmachine run elasticsearch-ci/bwc

@rjernst
Copy link
Member

rjernst commented Oct 23, 2019

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.

Copy link
Member

@danielmitterdorfer danielmitterdorfer left a comment

Choose a reason for hiding this comment

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

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.

@williamrandolph
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/default-distro
@elasticmachine run elasticsearch-ci/packaging-sample-matrix

@williamrandolph williamrandolph merged commit 1dd7ab8 into elastic:7.x Oct 25, 2019
@williamrandolph williamrandolph deleted the fix/7.x/default-max-heap-size-check branch October 25, 2019 17:27
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.
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants