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

[David Holmes]

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?


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