RFR: 8224878: Use JVMFlag parameters instead of name strings
Stefan Karlsson
stefan.karlsson at oracle.com
Wed May 29 12:51:44 UTC 2019
Thanks for reviewing!
StefanK
On 2019-05-28 23:22, John Rose wrote:
> Reviewed. This is a very good cleanup on
> a mini-framework which used to be simple
> and has grown hairy over the years.
>
> I think you should add an expression evaluator
> feature now to the flag framework. …Not.
> Just kidding! It's plenty complicated already.
> Thanks for making it more manageable.
>
> — John
>
> On May 28, 2019, at 10:15 AM, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
>>
>> To restrict the usages of flags fetched with find_flag_constrained (or whatever we'll call it), I changed it to return a const JVMFlag*. This way flags retrieved from that function won't be writable. If you want to write to a flag, you need to use the find_flag function, which does the appropriate checks (not locked, nor is_constant_in_binary).
>>
>> Updated webrevs:
>> https://cr.openjdk.java.net/~stefank/8224878/webrev.02.delta
>> https://cr.openjdk.java.net/~stefank/8224878/webrev.02
>>
>> Thanks,
>> StefanK
>>
>> On 2019-05-28 15:23, Stefan Karlsson wrote:
>>> 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
>>
>
More information about the hotspot-runtime-dev
mailing list