RFR 8067447: Factor out the shared implementation of the VM flags manipulation code
David Holmes
david.holmes at oracle.com
Thu Jan 15 07:23:27 UTC 2015
Hi Staffan
On 15/01/2015 2:38 AM, Staffan Larsen wrote:
>
>> On 14 jan 2015, at 08:50, David Holmes <david.holmes at oracle.com> wrote:
>>
>> Hi Staffan,
>>
>> On 13/01/2015 10:27 PM, Staffan Larsen wrote:
>>>
>>>> On 13 jan 2015, at 13:21, David Holmes <david.holmes at oracle.com
>>>> <mailto: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 <mailto: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?
>>
>> The DTrace flags are not manageable flags.
>
> I think that is a mistake. They should be. I do not understand why they are not - at the very least they should be “product_rw”. Perhaps product_rw did not exist when they were introduced?
product_rw is only for internal private flags. The DTrace flags are not
private, so Manageable seems like the way to go, taking into account:
// A flag can be made as "manageable" only if
// - the flag is defined in a CCC as an external exported interface.
// - the VM implementation supports dynamic setting of the flag.
// This implies that the VM must *always* query the flag variable
// and not reuse state related to the flag state at any given time.
// - you want the flag to be queried programmatically by the customers.
>
>> If they change to be manageable then they can be modified via ManagedFlag. But the second problem with these flags is that setting them after VM initialization you have to not only change the value of the flag, you have to call the additional methods like AttachListener does. Maybe that prevents them from being "manageable"? - otherwise ManagedFlags has to know what actions occur in arguments.cpp when the flag is set during VM startup.
>
> Yes, ManagedFlags should know what actions should be taken if a managed flag is changed. This is precisely why we want to introduce a ManagedFlags abstraction. Currently that logic is spread out in attachListener.cpp and management.cpp (see the code for “MaxHeapFreeRatio”). We want to consolidate it into one place (and in the future add jcmd as an additional way to change flags).
>
> Most managed flags don’t need an action to be taken when they are changed, but the Dtrace and MaxHeapFreeRatio flags do.
Not sure how you will use ManagedFlag in the context of argument
processing, to keep these actions in one place ??
Also this doesn't seem clear cut in the case of ExtendedDTraceProbes as
arguments.cpp simply does:
} else if (match_option(option, "-XX:+ExtendedDTraceProbes")) {
FLAG_SET_CMDLINE(bool, ExtendedDTraceProbes, true);
FLAG_SET_CMDLINE(bool, DTraceMethodProbes, true);
FLAG_SET_CMDLINE(bool, DTraceAllocProbes, true);
FLAG_SET_CMDLINE(bool, DTraceMonitorProbes, true);
whereas the attachListener code doesn't touch the actual
ExtendedDTraceProbes flag value but calls:
DTrace::set_extended_dprobes(flag);
which potentially sets the other three DTrace* flags and if it did that
it deoptimizes the world! None of which seems like code that ManagedFlag
should have any knowledge of. So at best I think the attachlistener code
might use the ManagedFlag abstraction rather than the direct calls to:
CommandLineFlags::boolAtPut((char*)flag, strlen(flag), &value,
Flag::ATTACH_ON_DEMAND);
??
> Perhaps “ManagedFlags” should change name to WriteableFlags? Would that make it’s purpose clearer? We do have flags marked both “manageble” and “product_rw” in globals.hpp that can be changed.
If you expand it to cover product_rw then a different name might be better.
>>
>> This is what I wrote in the email (which Jaroslav has not had a chance to respond to yet):
>>
>> The "pd" specific part of AttachListener set_flag is to allow the attach_listener code to set none-manageable flags (the Dtrace flags are product not manageable), and as such should not form part of the ManagedFlags code. And you misfactored that part because you have it on an else that will only happen if an invalid flag name is passed in:
>>
>> 184 Flag* f = Flag::find_flag((char*)flag_name, strlen(flag_name));
>> 185 if (f) {
>> 186 // only manageable flags are allowed to be set
>> 187 if (f->is_external() && f->is_writeable()) {
>> // ... set the flag
>> 204 } else {
>> 205 out->print_cr("Only 'manageable' flags can be set.");
>> 206 res = JNI_ERR;
>> 207 }
>> 208 } else {
>> // find_flag failed to find the flag
>> 209 res = pd_set_flag(flag_name, flag_value, origin, out);
>> 210 }
>>
>> whereas AttachListener has:
>>
>> Flag* f = Flag::find_flag((char*)name, strlen(name));
>> if (f && f->is_external() && f->is_writeable()) {
>> // set flag
>> } else {
>> return AttachListener::pd_set_flag(op, out);
>> }
>>
>> and this else part is for the non-manageable case (an invalid flag name will just be treated as an error by the pd_set_flag logic: "flag can not be changed").
>>
>> So ManagedFlags::set_flag should not have a pd_set_flag at all but just:
>>
>> Flag* f = Flag::find_flag((char*)flag_name, strlen(flag_name));
>> if (f) {
>> // only manageable flags are allowed to be set
>> if (f->is_external() && f->is_writeable()) {
>> // ... set the flag
>> } else {
>> out->print_cr("Only 'manageable' flags can be set.");
>> res = JNI_ERR;
>> }
>> } else {
>> out->print_cr("Flag not found");
>> res = JNI_ERR;
>> }
>>
>
> Yes, agree on the above.
Okay. I'll await a further webrev then. :)
>> And AttachListener::set_flag should look like:
>>
>> static jint set_flag(AttachOperation* op, outputStream* out) {
>>
>> const char* name = NULL;
>> if ((name = op->arg(0)) == NULL) {
>> out->print_cr("flag name is missing");
>> return JNI_ERR;
>> }
>>
>> if (ManagedFlags::set_flag(op->arg(0), op->arg(1),
>> Flag::ATTACH_ON_DEMAND, out) == JNI_ERR) {
>> // not a manageable flag - see if there is platform specific
>> // logic for this flag
>> return AttachListener::pd_set_flag(op, out);
>> }
>> else {
>> return JNI_OK;
>> }
>> }
>
> We should make the Dtrace flags into product_rw flags and then we can remove all the pd_set_flag() methods. I can’t see a reason for them.
As per above I don't think what attach listener does in this case is
really something that belong in ManagedFlags.
>>
>> The only issue with the above is that ManagedFlags::set_flag is going to write an error message to the outputstream, which is not wanted when processing a pd flag. In my opinion ManagedFlag::set_flag shouldn't be concerned with writing error messages as it doesn't know what constitutes an error. In which case the ManagedFlag::set_flag logic simplifies further, but AttachListener would have to do additional work itself if it want to produce error messages.
>
> ManagedFlags should know what the errors are and the errors should be propagated to attachListener (and management.cpp) so that they can be further propagated to the user.
If we no longer pass non-manageable flags that would be okay. The
problem I was alluding is where ManagedFlag writes an error "Not a
manageable flag" when it isn't actually an error.
> I would also want jmm_SetVMGlobal() in management.cpp to call ManagedFlag::set_flag directly instead of doing it’s own type-switching.
Haven't looked at that code so don't know how amenable it will be to this.
Thanks,
David
> Thanks,
> /Staffan
>
>
>>
>> ----
>>
>> Cheers,
>> David
>>
>>
>>> /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-
>>>
>
More information about the serviceability-dev
mailing list