Hi Staffan,
On 13/01/2015 10:27 PM, Staffan Larsen wrote:
On 13 jan 2015, at 13:21, David Holmes <david.hol...@oracle.com
<mailto: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 <mailto: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?
The DTrace flags are not manageable flags. 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.
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;
}
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;
}
}
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.
----
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-
