RFR: 8224878: Use JVMFlag parameters instead of name strings
Stefan Karlsson
stefan.karlsson at oracle.com
Wed May 29 15:49:56 UTC 2019
On 2019-05-29 15:56, gerard ziemski wrote:
> Impressive how you keep finding ways to improve this code.
:) Thanks.
The main reason why I started looking at this code is because I want to
add MinHeapSize, and maybe make it manageable. Whenever the flag is
changed we'd like to get a callback into the GC. That can almost be done
with the constraint functions, but that has some limitations that I
wanted to see if I could change. In the end, I think I can work with
what we have, but while looking at this I wanted to simplify some of the
code we had.
I have a few more prototypes and ideas that we might want to consider:
- Allow multiple constraint functions. It allows you to have one for
initialization, and another when a flag is set through the management
APIs.This is a fairly small change, after the latest cleanups.
- Replace a large portion of the code duplication by replacing type
specialized classes and functions with templates. I've prototyped this,
but not sure it's worth the effort.
- Replace all strcmp calls in the is_<type>() functions - This is a very
small and easy patch.
- Remove product_rw, which isn't used today
- Allow all types of flags to be managable (product, diagnostic,
experimental, develop) not only product flags. A fairly small patch, but
needs to get buy-in from others.
>
> Thanks for doing this.
Thanks,
StefanK
>
> On 5/29/19 7:51 AM, Stefan Karlsson wrote:
>> Hi David,
>>
>> Thanks for reviewing this! I've updated the patch with your suggested
>> name change:
>> http://cr.openjdk.java.net/~stefank/8224878/webrev.03.delta/
>> http://cr.openjdk.java.net/~stefank/8224878/webrev.03/
>>
>> Thanks,
>> StefanK
>>
>> On 2019-05-29 12:19, David Holmes wrote:
>>> Hi Stefan,
>>>
>>> This all looks okay to me - working with the Flag instead of the
>>> name seems a lot clearer.
>>>
>>> My only suggestion, as per other email, is to change
>>> find_flag_unconstrained to find_declared_flag. But up to you.
>>>
>>> Thanks,
>>> David
>>>
>>> On 29/05/2019 3:15 am, Stefan Karlsson 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