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

Tue Jan 13 12:27:17 UTC 2015

> On 13 jan 2015, at 13:21, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Staffan,
> 
> On 13/01/2015 5:26 PM, Staffan Larsen wrote:
>> 
>>> On 8 jan 2015, at 14:24, Jaroslav Bachorik <jaroslav.bachorik at oracle.com> wrote:
>>> 
>>> On 8.1.2015 12:12, David Holmes wrote:
>>>> 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.
>>> 
>>> I think I see the problem. Sorry it took me so long.
>>> 
>>> But, why the DTrace flags are only allowed to be set via the attachListener? Can we allow their manipulation via com.sun.Flag? Or they need to stay restricted to the attach mechanism only for whatever reason?
>> 
>> I don’t think there is any reason these Dtrace flags should only ba changeable by the attach mechanism. They could just as well be changed from JMX or jcmd. I guess the code is in attach because attach was the only way of changing flags at the time. The only difference I can see for these Dtrace flags compared to other flags is that some action needs to be taken if the flag is changed (calls to DTrace::set_extended_dprobes()). I also think the Dtrace flags should be marked as “manageable.”
> 
> I'm having a separate discussion with Jaroslav via email. The key thing here (and it is wrong in the refactor) is that the pd_set_flag in the AttachListener only exists to allow for non-manageable flags to be set. That functionality is specific to the AttachListener code and makes no sense for a ManagedFlag abstraction.

I don’t agree - or perhaps I am misunderstanding. I thought ManagedFlag was supposed to be the common way for attach, jcmd, jmx or any other future API to change flags in runtime. The DTrace flags aren’t special to AttachListener - they should be changeable from jmx and jcmd as well. Thus, the special handling of them should be in ManagedFlag, no?

/Staffan

> 
> David 
> 
>> /Staffan
>> 
>>> 
>>>> 
>>>>>> 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.
>>> 
>>> Bigger picture is that introducing yet another copy of the flag management code for the purpose of adding the "VM.set_flag" diagnostic command did seem unwieldy. The purpose of this refactoring was to get the shared parts to one place.
>>> 
>>> -JB-
>>> 
>>>> 
>>>> 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-

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20150113/86d2e8aa/attachment-0001.html>