RFR 8067447: Factor out the shared implementation of the VM flags manipulation code

[Staffan Larsen]

> On 9 feb 2015, at 00:13, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Jaroslav,
> 
> On 6/02/2015 9:54 PM, Jaroslav Bachorik wrote:
>> On 6.2.2015 06:16, David Holmes wrote:
>>> On 6/02/2015 1:17 AM, Jaroslav Bachorik wrote:
>>>> On 5.2.2015 14:55, Staffan Larsen wrote:
>>>>> Jaroslav,
>>>>> 
>>>>> This looks good, just a few small comments:
>>>>> 
>>>>> writableFlags.hpp:
>>>>> 
>>>>> L25 #ifndef WritableFlags_HPP
>>>>> L26 #define WritableFlags_HPP
>>>>> L96 #endif /* WritableFlags_HPP */
>>>>> Should be SHARE_VM_SERVICES_WRITABLEFLAG_HPP
>>>>> 
>>>>> L32: I don’t like inverted logic such as “NO_ERR”. I would prefer
>>>>> “SUCCESS” instead.
>>>>> 
>>>>> writableFlags.cpp:
>>>>> 
>>>>> L25: #include statements should be in alphabetical order if possible
>>>>> L166: (nit) missing empty line
>>>>> L155: I notice the Flag class uses “writeable” (with an ‘e’) and this
>>>>> class uses “writable” (without ‘e’) - My english isn’t good enough to
>>>>> tell if there is any difference.
>>>> 
>>>> They should be equal in meaning. According to GooglBattle [1] the form
>>>> 'writable' is far wider spread but I would leave the decision to a
>>>> native speaker.
>>>> 
>>>> [1]
>>>> http://www.googlebattle.com/index.php?domain=writeable&domain2=writable&submit=Go!
>>>> 
>>>>> L194: Unused variable “succeed"
>>>> 
>>>> The rest of the comments will be addressed. I will wait for eg. David to
>>>> comment on 'writable' vs. 'writeable' before regenerating the webrev.
>>> 
>>> Take your pick. Both are used in the hotspot sources eg globals.hpp
>>> contains is_writeable() and check_writable() :(
>>> 
>>> It is on my TODO list to review this ASAP.
>> 
>> Taffan's comments addressed -
>> http://cr.openjdk.java.net/~jbachorik/8067447/webrev.03
> 
> src/share/vm/services/writableFlags.cpp
> 
> precompiled.hpp must be first in the include list.
> 
> 73 int WritableFlags::set_uintx_flag(const char* name, uintx value, Flag::Flags origin, FormatBuffer<80>& err_msg) {
> 74   if (strncmp(name, "MaxHeapFreeRatio", 17) == 0) {
> 
> Strikes me that this flag specific validation does not belong in this logic (nor does it belong where it currently is today). Maybe the work being done on argument validation will allow this to be cleaned up in the near future?

At least now the logic is only in two places (writableFlags.cpp and arguments.cpp) instead of three places (management.cpp, attachListener.cpp and arguments.cpp). I, too, hope we can reduce this further.

/Staffan


> 
> 
> 154     // only writable flags are allowed to be set
> 155     if (f->is_writeable()) {
> 
> This tells me you maybe made the wrong choice for the spelling issue :)
> If we already have writeable associated with flags then maybe stick with it?
> 
> Otherwise seem okay.
> 
> Thanks,
> David
> 
>> -JB-
>> 
>>> 
>>> David
>>> 
>>> 
>>> 
>>>> -JB-
>>>> 
>>>>> 
>>>>> 
>>>>> Thanks,
>>>>> /Staffan
>>>>> 
>>>>>> On 4 feb 2015, at 17:55, Jaroslav Bachorik
>>>>>> <jaroslav.bachorik at oracle.com> wrote:
>>>>>> 
>>>>>> Hi, this is a round 2 review of the proposed solution.
>>>>>> 
>>>>>> Issue : https://bugs.openjdk.java.net/browse/JDK-8067447
>>>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8067447/webrev.02
>>>>>> 
>>>>>> This patch is a precursor for implementing
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8054890 which itself is a
>>>>>> part of JEP-228 (https://bugs.openjdk.java.net/browse/JDK-8043764).
>>>>>> 
>>>>>> Here, the code related to manipulating JVM flags is extracted to a
>>>>>> separate WritableFlags class and the codebease is adjusted to use
>>>>>> this class.
>>>>>> 
>>>>>> I didn't touch the original (nonstandard) handling of the DTrace
>>>>>> specific flags in the Solaris specific attachListener.cpp
>>>>>> implementation to keep the change simple and focused.
>>>>>> 
>>>>>> Thanks,
>>>>>> 
>>>>>> -JB-
>>>>> 
>>>> 
>>