[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR 8067447: Factor out the shared implementation of the VM flags manipulation code
From:       David Holmes <david.holmes () oracle ! com>
Date:       2015-02-09 11:17:26
Message-ID: 54D89746.5040603 () oracle ! com
[Download RAW message or body]

Hi Jaroslav,

// a writable flag

Still a few changes needed in writeable.hpp and writeable.cpp

But no need for a new webrev.

Thanks,
David

On 9/02/2015 8:48 PM, Jaroslav Bachorik wrote:
> 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@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-
>>>>>>
>>>>>
>>>
>
[prev in list] [next in list] [prev in thread] [next in thread]