[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    RE: RFR (M) 8214892: Delayed starting of debugging via jcmd
From:       "Langer, Christoph" <christoph.langer () sap ! com>
Date:       2018-12-12 11:58:08
Message-ID: 595e6087b590411b930a32e8f0a0d3b0 () sap ! com
[Download RAW message or body]

Hi,

pushed: http://hg.openjdk.java.net/jdk/jdk/rev/21dfea980e23

Best regards
Christoph

> -----Original Message-----
> From: Langer, Christoph
> Sent: Dienstag, 11. Dezember 2018 11:28
> To: Schmelter, Ralf <ralf.schmelter@sap.com>; Chris Plummer
> <chris.plummer@oracle.com>; serviceability-dev@openjdk.java.net
> Cc: Lindenmaier, Goetz <goetz.lindenmaier@sap.com>
> Subject: RE: RFR (M) 8214892: Delayed starting of debugging via jcmd
> 
> 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@sap.com>; Chris Plummer
> > <chris.plummer@oracle.com>; serviceability-dev@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@sap.com>; Chris Plummer
> > <chris.plummer@oracle.com>; serviceability-dev@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@openjdk.java.net>
> > On
> > > Behalf Of Schmelter, Ralf
> > > Sent: Donnerstag, 6. Dezember 2018 15:54
> > > To: Chris Plummer <chris.plummer@oracle.com>; serviceability-
> > > dev@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

[prev in list] [next in list] [prev in thread] [next in thread]