RFR (M) 8214892: Delayed starting of debugging via jcmd

Tue Dec 11 10:28:15 UTC 2018

Hi,

@Ralf: Thanks for updating the patch. I'm good with it now �� As there are no other opinions about the naming of the jdwp agent property, I'm fine with "onjcmd". It is the main use case.

@all: With me and Chris as reviewers, I think we are good to push. I'll do this tomorrow if I don't hear objections until then.

Thanks and best regards
Christoph

> -----Original Message-----
> From: Schmelter, Ralf
> Sent: Monday, December 10, 2018 2:55 PM
> To: Langer, Christoph <christoph.langer at sap.com>; Chris Plummer
> <chris.plummer at oracle.com>; serviceability-dev at openjdk.java.net
> Subject: RE: RFR (M) 8214892: Delayed starting of debugging via jcmd
> 
> Hi Christoph,
> 
> > I have thought about a more generic name for the option rather than
> > "onjcmd". What about "standby=y". That would indicate that the
> > debugging agent is on standby and can be triggered by whatever
> > means. What do others think?
> 
> I'm not sure making the name more generic is the right move, when it will
> probably be enabled via the jcmd in >95% of the cases.
> 
> > I'd prefer  if you write out either "Debugging has been started."
> > or "Debugging is already active.". I think this would make it more
> > clear for the user of the feature what actually happened.
> 
> OK, I've changed that.
> 
> > A better wording would be:
> > "Starts up the Java debugging if the jdwp agentlib was enabled with the
> option onjcmd=y."
> 
> OK.
> 
> > replace "debug triggered by jcmd?" with " start debug via jcmd"
> 
> Ok.
> 
> > Is it really necessary/desired to set suspendOnInit to false? We
> > could still honor suspend=y|n. The suspension will of course
> > happen at the time debug activation is triggered.
> 
> I don't think it would make sense to support suspend=y. In makes sense
> when starting the debugging directly (so no, or at least not much) Java code
> can be executed and you can debug from the beginning. And it also makes
> sense for the onthrow/onuncaught, since you can to see the exceptions in
> the debugger. But when triggered from the outside, you have no natural
> event to wait for and no state to preserve. The only effect you would see is
> that the jcmd would not return, since the initialize method would be blocked
> until a debugger has been connected.
> 
> I've updated the webrev:
> http://cr.openjdk.java.net/~rrich/webrevs/schmelter/8214892/webrev.03/
> 
> Best regards,
> Ralf Schmelter
> 
> 
> -----Original Message-----
> From: Langer, Christoph
> Sent: Freitag, 7. Dezember 2018 09:41
> To: Schmelter, Ralf <ralf.schmelter at sap.com>; Chris Plummer
> <chris.plummer at oracle.com>; serviceability-dev at openjdk.java.net
> Subject: RE: RFR (M) 8214892: Delayed starting of debugging via jcmd
> 
> Hi Ralf,
> 
> thanks for doing this change. Overall, it looks very good to me and seems to
> be a nice feature.
> 
> I have thought about a more generic name for the option rather than
> "onjcmd". What about "standby=y". That would indicate that the debugging
> agent is on standby and can be triggered by whatever means. What do
> others think?
> 
> Here are some minor comments, mostly about the wording of text
> messages:
> 
> src/hotspot/share/services/diagnosticCommand.cpp, line 1097:
> I'd prefer  if you write out either "Debugging has been started." or
> "Debugging is already active.". I think this would make it more clear for the
> user of the feature what actually happened.
> 
> src/hotspot/share/services/diagnosticCommand.hpp, line 878:
> "Starts up the Java debugging if enabled in the jdwp agentlib via the
> onjcmd=y option."
> A better wording would be:
> "Starts up the Java debugging if the jdwp agentlib was enabled with the
> option onjcmd=y."
> 
> src/jdk.jdwp.agent/share/native/libjdwp/debugInit.c:
> line 876:
> replace "debug triggered by jcmd?" with " start debug via jcmd" (or "start
> debug on request" if we opt to change the option name from onjcmd to
> something more general)
> line 1300:
>          suspendOnInit = JNI_FALSE;
> Is it really necessary/desired to set suspendOnInit to false? We could still
> honor suspend=y|n. The suspension will of course happen at the time debug
> activation is triggered.
> 
> Another question: Do we need a CSR for this?
> 
> Best regards
> Christoph
> 
> > -----Original Message-----
> > From: serviceability-dev <serviceability-dev-bounces at openjdk.java.net>
> On
> > Behalf Of Schmelter, Ralf
> > Sent: Donnerstag, 6. Dezember 2018 15:54
> > To: Chris Plummer <chris.plummer at oracle.com>; serviceability-
> > dev at openjdk.java.net
> > Subject: [CAUTION] RE: RFR (M) 8214892: Delayed starting of debugging via
> > jcmd
> >
> > Hi Chris,
> >
> > > I think I prefer passing EI_VM_INIT for triggering_ei, but if you prefer
> > > to keep it as is, I suggest a comment to clarify why it might be out of
> > > range.
> >
> > Actually, I used EI_VM_INIT for the longest time and only changed it
> > recently, because I thought that code could assume that e.g. no classes
> have
> > been loaded yet when getting the INIT_VM event. But since the JVMTI
> spec
> > does not guarantees this in any way (it allows other events to be send
> before
> > a VM_INIT), I just will change it back to use EI_VM_INIT for the initialize
> call.
> >
> > Regarding the name of the option, I agree that onjcmd, while not
> technically
> > fully accurate, makes most sense for the common user.
> >
> > > ... It think you could just return the error right away and remove
> > > the error checking code that comes later.
> >
> > I've changed the code to just return the error directly.
> >
> > > It's not clear to me why you want the name and address of the first
> > > transport. Why is the first one necessarily the one you want?
> >
> > Since currently the bag of transports must always have a size of 1, getting
> the
> > first or the last transport is the same. But the callback function used to
> iterate
> > the bag has to return a boolean value. I've decided to return JNI_FALSE,
> > which would mean the iteration stops at the first entry.
> >
> >
> > I've updated the webrev with your and Goetz Lindenmeier's suggestions:
> >
> http://cr.openjdk.java.net/~rrich/webrevs/schmelter/8214892/webrev.02/
> >
> > Best regards,
> > Ralf