> On 13 jan 2015, at 13:21, David Holmes <david.hol...@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.bacho...@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-
