> On 8 jan 2015, at 14:24, Jaroslav Bachorik <[email protected]>
> 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.”
/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-
>>>>>
>>>
>
