RFR: 8224980: FLAG_SET_ERGO silently ignores invalid values [v2]

[Ioi Lam]

On Tue, 14 Feb 2023 06:47:38 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Ioi Lam has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   @dholmes-ora comment - make sure errors are printed for FLAG_SET_ERGO
>
> src/hotspot/share/code/codeCache.cpp line 1197:
> 
>> 1195:   } else {
>> 1196:     // Use a single code heap
>> 1197:     FLAG_SET_ERGO(NonNMethodCodeHeapSize, (uintx)os::vm_page_size());
> 
> Sanity check: this code does execute after `os::init()` doesn't it?

Yes. In fact, `FLAG_SET_ERGO` internally calls the constraint function for this flag, `VMPageSizeConstraintFunc()`, which internally calls `os::vm_page_size()`.

> src/hotspot/share/compiler/compilerDefinitions.cpp line 134:
> 
>> 132: intx CompilerConfig::jvmflag_scaled_freq_log(intx freq_log) {
>> 133:   return MAX2((intx)0, MIN2(scaled_freq_log(freq_log), (intx)30));
>> 134: }
> 
> If the passed in arguments are out of range this appears to cap them within range but the initial error is never made known to the caller. ??

This code is called only by `CompilerConfig::set_compilation_policy_flags()` to scale 15 flags according to `CompileThresholdScaling`. If the user sets `CompileThresholdScaling` to be too high, then some of the calculated values will be capped.

@veresov @TobiHartmann - do you think some warning messages are needed here?

My thinking is that `CompileThresholdScaling` is a somewhat magical knob. If the user really wants to know what's happening, they should add `-XX:+PrintFlagsFinal` and see the effect on all the affected flags.

> src/hotspot/share/runtime/flags/jvmFlagAccess.cpp line 71:
> 
>> 69:       if (err != JVMFlag::SUCCESS) {
>> 70:         if (origin == JVMFlagOrigin::ERGONOMIC) {
>> 71:           fatal("FLAG_SET_ERGO cannot be used to set an invalid value for %s", flag->name());
> 
> Just occurred to me, should the `fatal` rather be a `vm_exit_during_initialization`?

`vm_exit_during_initialization` is usually for conditions not controllable by the VM (out of memory, bad command-line options, etc). Here were have a programming error in the VM's ergo code. So `fatal()` is more appropriate. It also generates an hs_err file that can help debugging.

> src/hotspot/share/runtime/globals_extension.hpp line 92:
> 
>> 90: 
>> 91: // FLAG_SET_ERGO must be always be called with a valid value. You cannot pass in a unvalidated
>> 92: // value and expect a return code for success/failure.
> 
> Suggestion:
> 
> // FLAG_SET_ERGO must be always be called with a valid value. If an invalid value
> // is detected then the VM will exit.

Fixed.

-------------

PR: https://git.openjdk.org/jdk/pull/12549