RFR: 8224878: Use JVMFlag parameters instead of name strings

[Stefan Karlsson]

Hi David,

On 2019-05-28 15:05, David Holmes wrote:
> Hi Stefan,
> 
> Just in relation to the find_flag(name) versus find_flag(name, len) 
> issue, I'm trying to see the simplest thing to "flip" so that it makes 
> more sense, but to be honest I don't understand why we use the 
> find_flag(name, len) form in the places that we do. For example:
> 
> JVMFlag::Error JVMFlag::boolAtPut(const char* name, size_t len, bool* 
> value, JVMFlag::Flags origin) {
>    JVMFlag* result = JVMFlag::find_flag(name, len);
>    return boolAtPut(result, value, origin);
> }
> 
> won't find a locked flag - but why not? I don't see why this should be a 
> function that only applies to unlocked flags. ??

The patch above actually removes that code:
-JVMFlag::Error JVMFlag::boolAtPut(const char* name, size_t len, bool* 
value, JVMFlag::Flags origin) {
-  JVMFlag* result = JVMFlag::find_flag(name, len);
-  return boolAtPut(result, value, origin);
-}

and left is the version that doesn't care if the flag is locked or not:
JVMFlag::Error JVMFlag::boolAtPut(JVMFlag* flag, bool* value, 
JVMFlag::Flags origin) {

However, now that you mention it, maybe that's unwise. Maybe we actually 
should check that we don't set locked (or is_constant_in_binary) flags.

> 
> But given we are talking about locked versus unlocked flags then 
> find_flag_unlocked() would seem more appropriate than 
> find_flag_unconstrained().

There's an extra dimension to this:

  896 // Search the flag table for a named flag
  897 JVMFlag* JVMFlag::find_flag(const char* name, size_t length, bool 
allow_locked, bool return_flag) {
  898   for (JVMFlag* current = &flagTable[0]; current->_name != NULL; 
current++) {
  899     if (str_equal(current->_name, current->get_name_length(), 
name, length)) {
  900       // Found a matching entry.
  901       // Don't report notproduct and develop flags in product builds.
  902       if (current->is_constant_in_binary()) {
  903         return (return_flag ? current : NULL);
  904       }
  905       // Report locked flags only if allowed.
  906       if (!(current->is_unlocked() || current->is_unlocker())) {
  907         if (!allow_locked) {
  908           // disable use of locked flags, e.g. diagnostic, 
experimental,
  909           // etc. until they are explicitly unlocked
  910           return NULL;
  911         }
  912       }
  913       return current;
  914     }
  915   }
  916   // JVMFlag name is not in the flag table
  917   return NULL;
  918 }

it's not only locked flags that get returned. See lines 902-903. Do you 
still think find_flag_unlocked would be a better name?

> 
> More tomorrow ...

Thanks,
StefanK

> 
> Cheers,
> David
> 
> On 28/05/2019 10:44 pm, Stefan Karlsson wrote:
>> And here's the webrev:
>>   http://cr.openjdk.java.net/~stefank/8224878/webrev.01/
>>
>> Thanks,
>> StefanK
>>
>> On 2019-05-28 13:35, Stefan Karlsson wrote:
>>> Hi all,
>>>
>>> Please review this patch to use JVMFlag parameters instead of name 
>>> strings in the JVM flag handling code.
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8224878
>>>
>>> The intention is to reduce the places where the API uses "const char* 
>>> name" parameters to describe a JVM flag, and instead use a JVMFlag* 
>>> when performing various flag handling code. The places where we 
>>> translate from "const char* name" to JVMFlag* has been pushed to the 
>>> outer layers of the code, which explicitly uses the 
>>> JVMFlag::find_flag functions.
>>>
>>> This allows us to replace strcmp with simple JVMFlag pointer equality 
>>> checks in jvmFlagRangeList.cpp and jvmFlagConstraintList.cpp.
>>>
>>> This gets rid of the need to store a typed pointer to the flag in 
>>> jvmFlagRangeList.cpp and jvmFlagConstraintList.cpp.
>>>
>>> This gets rid of the the need to propagate the 'allow_locked' and 
>>> 'return_flag' parameters down the call chain. For example, see the 
>>> changes to JVMFlag::<type>At and JVMFlag::<type>AtPut:
>>> -  static JVMFlag::Error intAt(const char* name, int* value, bool 
>>> allow_locked = false, bool return_flag = false)      { return 
>>> intAt(name, strlen(name), value, allow_locked, return_flag); }
>>> +  static JVMFlag::Error intAt(const JVMFlag* flag, int* value);
>>>    static JVMFlag::Error intAtPut(JVMFlag* flag, int* value, 
>>> JVMFlag::Flags origin);
>>> -  static JVMFlag::Error intAtPut(const char* name, size_t len, int* 
>>> value, JVMFlag::Flags origin);
>>> -  static JVMFlag::Error intAtPut(const char* name, int* value, 
>>> JVMFlag::Flags origin)   { return intAtPut(name, strlen(name), value, 
>>> origin); }
>>>
>>>   It changes the JVMFlag::find_flag API. The way it uses default 
>>> values have a surprising effect for users of those functions. 
>>> find_flag(name) searches among all flags (even locked and constants), 
>>> while find_flag(name, strlen(name)) doesn't return locked or 
>>> constants flags. To make it less likely to accidentally call the 
>>> wrong version, this has been changed to the following:
>>> * JVMFlag::find_flag(name) - Fetches the flag if it is available 
>>> (unlocked and not constant).
>>> * JVMFlag::find_flag_unrestricted(name) - Fetches the flag even if it 
>>> is locked or a constant.
>>>
>>> Removed the unused JVMFlag::wasSetOnCmdline.
>>>
>>> Small cleanups
>>> - Renamed address_of_flag to flag_from_enum
>>> - Renamed local JVMFlag* variable from results to flag
>>> - Use initializer lists
>>> - Removed superfluous semicolons
>>>
>>> I don't mind splitting patch up into more than one RFE, if that seems 
>>> more appropriate.
>>>
>>> Tested with:
>>> - test/hotspot/jtreg/runtime/CommandLine
>>> - tier1 (tier2-3 80% done)
>>>
>>> Suggestions on other testing is appreciated.
>>>
>>> Thanks,
>>> StefanK