Fix ignored env var options #244
Conversation
This PR lets MMTk-side Options read environment variables for options after OpenJDK-side options are applied. By doing this, environment variables like MMTK_THREADS are no longer ignored.
openjdk/mmtkHeap.cpp
Outdated
| } | ||
|
|
||
| // Read MMTk options from environment variables (such as `MMTK_THREADS`), overriding cmdline options. | ||
| mmtk_builder_read_env_var_settings(); |
There was a problem hiding this comment.
The bug in #244 is that the env var value got overwritten by the command line value. But with the change, the command line value will get overwritten by the env var value. So the PR does not solve the issue, it just changes the priority of the option value from different sources.
We should not mix the use of env vars and command line options. Maybe it is time to stop using env vars. It may take some time for the scripts and running configs to keep up with the change. I guess we could make it a goal for the next release. We would have 6 weeks to fix broken stuff.
mmtk/mmtk-core#955 is good. It allows us to disable setting options by env var easily. We could also accept mmtk/mmtk-core#954. It is a workaround for the immediate issue #244. In the long run, it is also okay to allow users to check if an option is explicitly set or not.
There was a problem hiding this comment.
In short, my suggestion is we review both mmtk-core PRs, and we move away from env var in the next release.
There was a problem hiding this comment.
Actually I believe this solves the issue. For openjdk, there are three ways to set options: (1) MMTK_ env vars (2) ThirdPartyHeapOptions (3) Copied from hotspot cmd args
The problem is that we're ignoring env var values and unconditionally using openjdk cmd arg for the threads option. This should be solved as long as we define a clear priority between these three sources, and make sure the one with higher priority can overwrite others, instead of ignoring it.
There was a problem hiding this comment.
Tbh I'm not sure why you can't set ParallelGCThreads for OpenJDK instead of MMTK_THREADS in #242. Is there a particular reason we need to support that?
There was a problem hiding this comment.
Actually I believe this solves the issue. For openjdk, there are three ways to set options: (1)
MMTK_env vars (2) ThirdPartyHeapOptions (3) Copied from hotspot cmd argsThe problem is that we're ignoring env var values and unconditionally using openjdk cmd arg for the
threadsoption. This should be solved as long as we define a clear priority between these three sources, and make sure the one with higher priority can overwrite others, instead of ignoring it.
OK. I guess the actual issue is that command line args always have a default value, so when we prioritise command line args, it always overwrites values from other sources. But if we prioritise env vars, it could be just empty if not set. If you think mmtk/mmtk-core#954 and the OpenJDK PR is not needed, you can close them.
There was a problem hiding this comment.
Tbh I'm not sure why you can't set ParallelGCThreads for OpenJDK instead of MMTK_THREADS in #242. Is there a particular reason we need to support that?
ParallelGCThreads should work. The problem is we can only set ParallelGCThreads now. The binding will ignore the values in MMTK_THREADS
There was a problem hiding this comment.
OK. I guess the actual issue is that command line args always have a default value, so when we prioritise command line args, it always overwrites values from other sources. But if we prioritise env vars, it could be just empty if not set.
Yes. That's the cause. But OpenJDK actually provides a way to tell whether a command line option is explicitly given or omitted as default (FLAG_IS_DEFAULT(ParallelGCThreads)). I have adjusted the PR so that environment variables overrides the default of ParallelGCThreads, but if -XX:ParallelGCThreads=xxxx is given on the command line, it will override the environment variable instead.
If you think mmtk/mmtk-core#954 and the OpenJDK PR is not needed, you can close them.
But after a second thought, mmtk/mmtk-core#954 may be useful if we want to optionally pass non-default MMTk options to other places just like we optionally pass non-default OpenJDK options to MMTk. We can keep both mmtk/mmtk-core#954 and mmtk/mmtk-core#955.
There was a problem hiding this comment.
Tbh I'm not sure why you can't set ParallelGCThreads for OpenJDK instead of MMTK_THREADS in #242. Is there a particular reason we need to support that?
ParallelGCThreadsshould work. The problem is we can only setParallelGCThreadsnow. The binding will ignore the values inMMTK_THREADS
That's what I'm saying, I'm not sure what the problem is with using ParallelGCThreads? Shouldn't we try to be as similar to the VM as possible? MMTK_THREADS and other environment variables are (still) there to quickly bootstrap new bindings/ports so that they don't have to bother fiddling with parsing commandline options.
|
I figured out that we can use |
|
|
||
| You can also set those options via command line arguments. | ||
|
|
||
| - `-XX:ParallelGCThreads=n` (where `n` is a number) sets the number of GC worker threads. |
There was a problem hiding this comment.
Could you clarify that this option actually changes both the number of STW/parallel GC threads and the concurrent GC threads for MMTk?
For OpenJDK GCs, -XX:ParallelGCThreads=n only changes the number of STW threads IIRC.
There was a problem hiding this comment.
Currently concurrent GC has not been merged into mmtk-core. So when Wenyu merges his branch, we can introduce the same interface (no of concurrent GC threads) to make it similar to the OpenJDK interface
There was a problem hiding this comment.
I added clarification about -XX:ConcGCThreads. Because we haven't merged concurrent plans, yet, I just mentioned that ConcGCThreads is ignored when using MMTk. When we merge concurrent plans, we can update this line of documentation, and maybe also adjust the MMTk options (such as introducing Options::concurrent_threads, etc.) that matches the OpenJDK options.
5 participants
This PR lets MMTk-side Options read environment variables for options after OpenJDK-side options are applied. By doing this, environment variables like MMTK_THREADS are no longer ignored.
Fixes: #242
This PR makes use of the change introduced in mmtk/mmtk-core#955