JVM Flag ergonomics and constraints

Tue Feb 7 05:53:08 UTC 2023

On 2/6/2023 4:48 PM, David Holmes wrote:
> 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);
> }
>
>

I don't think this coding style is useful. None of the existing code is 
structured this way.

Instead of arguing abstractly, let's say we are calling FLAG_SET_ERGO to 
set the InitialHeapSize.

The code that ergonomically sets InitialHeapSize should know the valid 
range for this flag. It should be in same module of code that implements 
the range constraint function for the InitialHeapSize. This module 
should use its own knowledge to choose a valid value for 
FLAG_SET_ERGO(InitialHeapSize).

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

That's not true. The ERGO setter of InitialHeapSize will use an 
algorithm that calculates a value that would be guaranteed to fit it the 
allowed range. The algorithm itself doesn't necessarily have to call the 
range check function:

     size_t n = calculate_init_heap();
     FLAG_SET_ERGO(InitialHeapSize, n);  // 
assert(InitialHeapSizeConstraintFunc(n, false) == JVMFlag::SUCCESS, ...);

The assert inside FLAG_SET_ERGO is exactly what the caller wants -- to 
ensure that its algorithm has produced a correct value.

Even if the ERGO setter really wants to call the range checker function, 
it can already do so:

    size_t n = calculate_init_heap();
    if (InitialHeapSizeConstraintFunc(n, false) != JVMFlag::SUCCESS) {
      n = .....;
    }
    FLAG_SET_ERGO(InitialHeapSize, n);

But we shouldn't dictate this style through the FLAG_SET_ERGO API.

Also, your proposal will not allow FLAG_SET_ERGO to assert. This means 
that all existing calls of FLAG_SET_ERGO (over 150 of them) need to be 
rewritten to catch the problem reported in JDK-8224980:

     JVMFlag::Error err = FLAG_SET_ERGO(flag, newVal);
     assert(err == JVMFlag::SUCCESS, "sanity");

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