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

[David Holmes]

On 8/01/2015 7:22 PM, Jaroslav Bachorik wrote:
> On 8.1.2015 03:45, David Holmes wrote:
>> On 8/01/2015 1:59 AM, Jaroslav Bachorik wrote:
>>> On 7.1.2015 02:31, David Holmes wrote:
>>>> On 17/12/2014 8:19 PM, Jaroslav Bachorik wrote:
>>>>> Please, review the following change.
>>>>>
>>>>> Issue : https://bugs.openjdk.java.net/browse/JDK-8067447
>>>>> Webrev: http://cr.openjdk.java.net/~jbachorik/8067447/webrev.00
>>>>>
>>>>> 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 ManagedFlags class and the codebease is adjusted to use this
>>>>> class.
>>>>
>>>> Not clear to me what this is addressing exactly - do we really need
>>>> platform specific variants of "set flag" ??
>>>
>>> It has just been moved from the corresponding attachListener_<os>.cpp
>>> files. And it is 'pd_set_flag' what, I suppose, means "platform
>>> dependent"?
>>
>> Yes it does and it mostly made sense inside the already pd
>> attachListener implementations, but it isn't obvious to me that it makes
>> sense in the ManagedFlag context. It is the choice about whether the
>> flag can be changed that is "pd" not the actual setting and those
>> choices are inherent to the attachListener mechanism they are not
>
> Why would the ability to set Solaris specific flags via DTrace be
> specific to the attachListener mechanism? Also, AFAICS here it is the
> mechanism of setting the flag which is "pd" and not the choice
> (DTrace::* vs CommandLineFlags::*)

The attachListener allows for manipulating VM flags if the attach 
mechanism is used. In the Solaris case it turns on some DTrace flags. 
The attach mechanism factors that into a pd_set_flags method that is 
called for a given AttachOperation and so allows per platform behaviour. 
But this is all part of the attach mechanism it isn't part of some 
general flag management process.

>> inherent to ManagedFlags - so this refactoring seems wrong to me. What
>> exactly is ManagedFlag supposed to represent?
>
> A shared functionality between attachListener.cpp, management.cpp and
> the new diagnostic commands to be introduced later (as mentioned in the
> original synopsis of this RFR). It did seem preferable over just copying
> the implementation over to a few more places.

I need to see a clearer bigger picture. What I currently see doesn't 
look right to me - attach mechanism functionality doesn't belong in a 
general purpose ManagedFlags abstraction.

David
-----

>>
>>>>
>>>> All the new code seems incorrect:
>>>>
>>>> jint ManagedFlags::pd_set_flag(const char* flag_name, const char*
>>>> flag_value, Flag::Flags origin, outputStream* out) {
>>>>     out->print_cr("flag '%s' cannot be changed", op->arg(0));
>>>>     return JNI_ERR;
>>>>   };
>>>>
>>>> op->arg(0) comes from the original code where op was an
>>>> AttachOperation*. Here is should be using flag_name.
>>>
>>> Correct. Slipped through and then replicated :/
>>
>> And obviously never compiled. RFRs should be for tested code.
>
> Yes, one should run always "make clean" first, just in case. I should
> remember this well to prevent further embarrassment.
>
> -JB-
>
>>
>> David
>> -----
>>
>>> Updated webrev: http://cr.openjdk.java.net/~jbachorik/8067447/webrev.01
>>>
>>> -JB-
>>>
>>>>
>>>> David
>>>>
>>>>
>>>>> Thanks,
>>>>>
>>>>> -JB-
>>>
>