JVM Flag ergonomics and constraints

[Ioi Lam]

On 2/5/2023 11:45 PM, David Holmes wrote:
> On 6/02/2023 5:26 pm, Ioi Lam wrote:
>> On 2/3/2023 12:40 AM, David Holmes wrote:
>>> On 3/02/2023 5:12 pm, Ioi Lam wrote:
>>>>
>>>>
>>>> On 2/1/2023 10:37 PM, David Holmes wrote:
>>>>> <trimming>
>>>>>
>>>>> On 2/02/2023 3:25 pm, Thomas Stüfe wrote:
>>>>>> On Thu, Feb 2, 2023 at 5:30 AM David Holmes 
>>>>>> <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
>>>>>>
>>>>>>     That would have to be determined case-by-case. For each ergo 
>>>>>> flag we
>>>>>>     would have to decide what value it is calculating and based 
>>>>>> on what,
>>>>>>     and
>>>>>>     determine whether an out-of-range value is an error or 
>>>>>> something to be
>>>>>>     wary of.
>>>>>>
>>>>>> Sure, what I meant was how to control it? Two sets of 
>>>>>> FLAT_SET_... macros, one that asserts on constraint violation, 
>>>>>> one that caps?
>>>>>
>>>>> No I expect to just see the places where FLAG_SET_ERGO is called 
>>>>> do the right thing by checking the return value and proceeding 
>>>>> accordingly.
>>>>
>>>> David, are you proposing this?
>>>>
>>>>     int value = calculate_ergo();
>>>>     if (FLAG_SET_ERGO (TheFlag, value) == error) {
>>>>       value = do_it_again()
>>>>       if (FLAG_SET_ERGO (TheFlag, value) == error) {
>>>>          huh?
>>>>       }
>>>>     }
>>>>
>>>> In the above code, all FLAG_SET_ERGO does is to check the range for 
>>>> you. It doesn't tell you why the range check fails. So the caller 
>>>> would have to redo the calculation (with no more information, other 
>>>> than that the last value was no good). What if the new value is 
>>>> also no good?
>>>
>>> To me that indicates we don't know what the heck we are calculating! 
>>> How can you calculate a setting for a flag if you don't even 
>>> understand the valid range of that flag. Which may mean the max, for 
>>> example, needs to be exposed programmatically.
>>>
>>>> With this style, you almost have to wrap every FLAG_SET_ERGO  in a 
>>>> loop, keep trying (in the dark) until you succeed.
>>>>
>>>> ==================
>>>> I think it's better to just assert inside FLAG_SET_ERGO if the 
>>>> value is out of range. The caller should make sure it has 
>>>> calculated a correct value.
>>>
>>> If the calculation uses some externally settable value then an 
>>> assert won't help if we're in a product build.
>>>
>>
>> We have lots of set_xxx() APIs in various classes that assert when 
>> given an inappropriate value -- this indicates a programming error. 
>> So FLAG_SET_ERGO would be no different.
>>
>> It's the responsibility of the caller of FLAG_SET_ERGO to pass a 
>> correct value to FLAG_SET_ERGO . How the caller comes up with that 
>> value (whether it depends on some external input) is irrelevant.
>> ========
>>
>> To properly catch the programming errors with ERGO setting, we should:
>>
>> - Change FLAG_SET_ERGO to return "void". The current API gives the 
>> false impression that the caller needs to check the returned value 
>> and do something about it.
>
> That's because they do! You still need to be able to report an error 
> for product builds if the value is being calculated based on some 
> passed in runtime value.
>

That should be done by the code that calls FLAG_SET_ERGO, not by 
FLAG_SET_ERGO itself. E.g., if an environment variable would cause an 
invalid ERGO setting, the code that handles this variable should report 
an error and should NOT call FLAG_SET_ERGO with an invalid value.

FLAG_SET_ERGO knows nothing about such env variables and cannot print 
out a meaningful error message.



> Putting in an assert is fine but not sufficient.
>
> David
> -----
>
>> - Assert if the value is out of range, so we can catch the 
>> programming error during testing (with debug builds)
>>
>> It's up to the caller of FLAG_SET_ERGO to determine a value that's 
>> within the allowed range.
>>
>> If the caller wants to query the constraints of the flag that it 
>> wants to set, that can be done via the JVMFlagLimit API. Using 
>> JVMFlagLimit is much better than blindly setting a value and getting 
>> an error code that doesn't tell you why it failed.
>>
>>
>>> As I said it depends on exactly what we are using to do the 
>>> calculation. If it is "closed world" then it is a fatal error to get 
>>> it wrong and should be detected during testing. Otherwise the code 
>>> has to deal with the possibility of error.
>>>
>>>>
>>>> Maybe we can add a new FLAG_SET_ERGO_CAPPED() that would cap the 
>>>> value, but that should be an opt-in. FLAG_SET_ERGO() should not 
>>>> automatically cap the value.
>>>
>>> That might be suitable for some flags/calculations.
>>>
>>> I don't like trying to talk about this in generalities. Each 
>>> specific situation needs to be examined to understand what is being 
>>> calculated and how. But it is the callers of FLAG_SET_ERGO that need 
>>> to know what they are doing.
>>>
>>> Cheers,
>>> David
>>>
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>