RFR: 8224878: Use JVMFlag parameters instead of name strings
David Holmes
david.holmes at oracle.com
Tue May 28 22:17:49 UTC 2019
Hi Stefan,
Quick response to your response, not the updated webrev.
This is even more complex than I had imagined but I now have a better
handle on the existing code. Comments below ...
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).
Okay that makes sense based on my new understanding.
>
> Updated webrevs:
> https://cr.openjdk.java.net/~stefank/8224878/webrev.02.delta
> https://cr.openjdk.java.net/~stefank/8224878/webrev.02
I haven't looked at this yet, I needed to clarify existing code usage
discussed below.
> 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.
So "allow_locked" and "return_flag" did not quite mean what I thought -
see below. The existing code will find any flag that can be written to
using find_flag(name, len) - which is exactly what it should do. It's
just not obvious that is what it is doing.
>>
>>>
>>> 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 }
Okay these allow_locked and return_flag parameters are starting to make
sense to me now. The "return_flag" covers the constant-in-binary case.
If false then such "flags" are not seen because they aren't flags they
are constants. If set true it says always return this flag even though
it is constant. It should be called return_constant_flags.
>> 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;
So allow_unlocked doesn't mean "find flags that need to be unlocked to
be used", it means "find flags that need to be unlocked to be used AND
which have NOT been unlocked". If a flag is already unlocked then it is
treated like any other flag and allow_unlocked is not relevant.
>> 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?
Not now that I understand what allow_unlocked actually means.
The existing API is actually making much more sense to me now - though
the overloads really don't convey the semantics:
find_flag(name): finds any flag of the given name regardless of whether
the flag is active (has been unlocked) or is non-constant. In short it
sees if we have defined a flag by that name. So lets call it
find_declared_flag(name).
find_flag(name, len) only finds "active flags" - flags that have
actually been enabled (if necessary e.g. for locked flags) and which are
true flags not constants. Using a specific overload to make this
distinction is truly awful from an API design perspective. But if we now
have find_declared_flag(name) then this one can simply be
find_flag(name) and it means "find an 'active' flag by this name".
That all said I'll go and take a look at your webrev later this morning.
Thanks,
David
------
>>
>>>
>>> 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