JVM Flag ergonomics and constraints

[David Holmes]

On 7/02/2023 3:19 am, Ioi Lam wrote:
> 
> 
> 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.

I'm not asking FLAG_SET_ERGO to report a specific error, just that the 
supplied value was out-of-range. Your position, as stated, is that the 
caller should validate everything before calling FLAG_SET_ERGO. My 
position was that we could still support a usage pattern where we assume 
everything will be fine and we recover if it is not e.g.

newVal = simpleCalc(x, y, z); // works 99% of time
if (!FLAG_SET_ERGO(flag, newVal)) {
   newVal = complexCalc(x, y, z); // guarantees in range
   FLAG_SET_ERGO(flag, newVal);
}

With what you propose the caller has to range-check newVal, and then 
FLAG_SET_ERGO will range-check it again.

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

I'm not wanting it to print anything.

David
-----

> 
> 
>> 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
>>>>>
>>>
>