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

[Jaroslav Bachorik]

On 9.2.2015 00:13, David Holmes 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.

done.

>
> 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?

i hope so. slowly getting there - instead of 3 different places 
performing this validation we have 2 now.

>
>
>   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?

renamed to 'writeable'

http://cr.openjdk.java.net/~jbachorik/8067447/webrev.04

-JB-

>
> 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-
>>>>>
>>>>
>>