Remove manual parsing of JVM options #41962
Conversation
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 yet another JVM 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.
jasontedor
added
>enhancement
:Delivery/Packaging
|
Pinging @elastic/es-core-infra |
|
To my beloved reviewers: This approach was initially suggested on the original work introducing the ergonomics. Please see the discussion there as part of your review before we consider pulling this in. I think that we can revisit the discussion here if needed. |
jasontedor
added
:Core/Infra/Core
|
This seems like a very heavyweight hack. Launching a VM with Xmx pretouched, just to find out what the value was is a huge cost (relative to normal startup). Both the bugs you mentioned (first vs last wins, missing ES_JAVA_OPTS) seem fixable with the current implementation. IMO keeping our own parsing isn't that expensive vs incurring the added startup cost for every node we start (which amplifies in our integration tests as we start a lot of nodes...). |
|
I think there is confusion here, introduced by my commit message (I will reword it); I’m sorry for that. If you refer back to the initial discussion in #30684, we do not actually start a JVM here, only invoke While what we are doing today has bugs that can be fixed without resorting to this, there are bugs we have today that can not. Here’s a list of the bugs we have today:
The third of these simply can not be fixed by us. My change fixes all of these, with no additional effort on our end. We don’t have to have a side implementation of the JVM parsing logic. Additionally, this sets us up to easily extract the value of Thus I maintain this is a superior approach. |
The semantics of invoking
I'm personally in favor of this approach. |
You are right, I completely missed that, I'm sorry. I misinterpreted a comment from Daniel in the previous discussion about startup time of java with 8GB Xmx. Given my new understanding, this approach does sound superior. |
…rsing * elastic/master: [ML] relax set upgrade mode test to match what is guaranteed (elastic#41958) Add note about ILM action ordering (elastic#41771) Remove Version 6 pre-release constants (elastic#41517) Mute illegal interval rollup tests Add static section whitelist info to api docs generation (elastic#41870) Cleanup RollupSearch exceptions, disallow partial results (elastic#41272)
There was a problem hiding this comment.
I'm fine with the approach. Indeed it seems superior to me.
Additionally, this sets us up to easily extract the value of MaxDirectMemorySize, without having to parse that too.
I think you left out one detail that led me to misinterpret your statement originally. We can properly parse MaxDirectMemorySize provided that the user has specified one. If the user did not specify one, we see a MaxDirectMemorySize of 0 because the JVM determines this value in a much later stage during startup. If I run this:
daniel@io:~ $ java -XX:+PrintFlagsFinal -version | grep MaxDirectMemorySize
I get:
uint64_t MaxDirectMemorySize = 0 {product} {default}
openjdk version "12" 2019-03-19
OpenJDK Runtime Environment (build 12+32)
OpenJDK 64-Bit Server VM (build 12+32, mixed mode, sharing)
But as I said: I think your intention was to state that we can parse the user-specified value with this approach as well.
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.
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.
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.
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.
Relates #30684