RFR(s): 8243451: nsk.share.jdi.Debugee.isJFR_active() is incorrect and corresponsing logic seems to be broken
Fairoz Matte
fairoz.matte at oracle.com
Wed Jun 10 06:00:23 UTC 2020
Hi Leonid,
The call isJFRActive() need to be executed on HeapwalkingDebuggee side.
This is what my understanding is.
Thanks,
Fairoz
> -----Original Message-----
> From: Leonid Mesnik
> Sent: Wednesday, June 10, 2020 1:16 AM
> To: Serguei Spitsyn <serguei.spitsyn at oracle.com>; Fairoz Matte
> <fairoz.matte at oracle.com>; Erik Gahlin <erik.gahlin at oracle.com>
> Cc: serviceability-dev at openjdk.java.net
> Subject: Re: RFR(s): 8243451: nsk.share.jdi.Debugee.isJFR_active() is incorrect
> and corresponsing logic seems to be broken
>
> http://cr.openjdk.java.net/~fmatte/8243451/webrev.09/test/hotspot/jtreg/vmT
> estbase/nsk/share/jdi/TestDebuggerType2.java.udiff.html
>
> I see that isJFRActive() depends on "nsk.share.jdi.HeapwalkingDebuggee".
> It is not going to work of debugee is not "nsk.share.jdi.HeapwalkingDebuggee".
>
> Shouldn't it be placed in HeapWalkingDebugger?
>
> Leonid
>
> On 6/8/20 9:26 PM, serguei.spitsyn at oracle.com wrote:
> > Hi Fairoz,
> >
> > LGTM.
> >
> > Thanks,
> > Serguei
> >
> >
> > On 6/8/20 21:20, Fairoz Matte wrote:
> >> Hi Serguei,
> >>
> >> Thanks for the clarifications,
> >> I have incorporated the 2nd suggestion, below is the webrev,
> >> http://cr.openjdk.java.net/~fmatte/8243451/webrev.09/
> >>
> >> Thanks,
> >> Fairoz
> >>
> >> From: Serguei Spitsyn
> >> Sent: Monday, June 8, 2020 10:34 PM
> >> To: Fairoz Matte <fairoz.matte at oracle.com>; Erik Gahlin
> >> <erik.gahlin at oracle.com>
> >> Cc: serviceability-dev at openjdk.java.net
> >> Subject: Re: RFR(s): 8243451: nsk.share.jdi.Debugee.isJFR_active() is
> >> incorrect and corresponsing logic seems to be broken
> >>
> >> Hi Fairoz,
> >>
> >>
> >> On 6/8/20 02:08, mailto:serguei.spitsyn at oracle.com wrote:
> >> Hi Fairoz,
> >>
> >> There are two different isJFRActive() methods, one is on debuggee
> >> side and another on the debugger side.
> >> The one on debuggee side is better to keep in Debuggee.java (where it
> >> was before) instead of moving it to HeapwalkingDebuggee.java.
> >> It is okay to keep the call to it in the HeapwalkingDebuggee.java.
> >>
> >> Please, skip this suggestion as Debugger.java is not one of supers of
> >> HeapwalkingDebuggee.java as I've assumed.
> >>
> >> Thanks,
> >> Serguei
> >>
> >>
> >> + protected boolean isJFRActive() {
> >> + boolean isJFRActive = false;
> >> + ReferenceType referenceType =
> >> debuggee.classByName("nsk.share.jdi.HeapwalkingDebuggee");
> >> + if (referenceType == null)
> >> + throw new RuntimeException("Debugeee is not initialized
> >> yet");
> >> +
> >> + Field isJFRActiveFld =
> >> referenceType.fieldByName("isJFRActive");
> >> + isJFRActive =
> >> ((BooleanValue)referenceType.getValue(isJFRActiveFld)).value();
> >> + return isJFRActive;
> >> }
> >> It is better to remove the line:
> >> + boolean isJFRActive = false;
> >> and just change this one:
> >> + boolean isJFRActive =
> >> ((BooleanValue)referenceType.getValue(isJFRActiveFld)).value();
> >>
> >> Otherwise, it looks good to me.
> >> I hope, it really works now.
> >>
> >> Thanks,
> >> Serguei
> >>
> >> On 6/8/20 00:26, Fairoz Matte wrote:
> >> Hi Serguei, Erik,
> >> Thanks for the reviews,
> >> Below webrev contains the suggested changes,
> >> http://cr.openjdk.java.net/~fmatte/8243451/webrev.08/
> >> The only thing I couldn’t do is to keep the local copy of
> >> isJFRActive() in HeapwalkingDebugger, The method is called in debugee
> >> code.
> >> In debugger, we have access to debugee before test started or after
> >> test completes.
> >> isJFRActive() method need to be executed during the test execution.
> >> Hence I didn’t find place to initialize and cannot make local copy.
> >> Thanks,
> >> Fairoz
> >> From: Serguei Spitsyn
> >> Sent: Tuesday, June 2, 2020 7:57 AM
> >> To: Fairoz Matte mailto:fairoz.matte at oracle.com; Erik Gahlin
> >> mailto:erik.gahlin at oracle.com
> >> Cc: mailto:serviceability-dev at openjdk.java.net
> >> Subject: Re: RFR(s): 8243451: nsk.share.jdi.Debugee.isJFR_active() is
> >> incorrect and corresponsing logic seems to be broken
> >> On 6/1/20 12:30, mailto:serguei.spitsyn at oracle.com wrote:
> >> Hi Fairoz,
> >>
> >> It looks okay in general.
> >> But I'm not sure this check is going to work.
> >> The problem is the HeapwalkingDebuggee.useStrictCheck method is
> >> invoked in the context of the HeapwalkingDebugger process, not the
> >> HeapwalkingDebuggee process.
> >>
> >> Probably, you wanted to get this bit of information from the Debuggee
> >> process.
> >> The debuggee has to evaluate it itself and store in some field.
> >> The debugger should use the JDI to get this value from the debuggee.
> >>
> >> Thanks,
> >> Serguei
> >>
> >> I'm not sure, what exactly you wanted to do here.
> >> It can occasionally work for you as long as both processes are run
> >> with the same options.
> >>
> >> Thanks,
> >> Serguei
> >>
> >>
> >> On 6/1/20 08:52, Fairoz Matte wrote:
> >> Hi Erik,
> >> Thanks for the review, below is the updated webrev.
> >> http://cr.openjdk.java.net/~fmatte/8243451/webrev.02/
> >> Thanks,
> >> Fairoz
> >> -----Original Message-----
> >> From: Erik Gahlin
> >> Sent: Monday, June 1, 2020 4:26 PM
> >> To: Fairoz Matte mailto:fairoz.matte at oracle.com
> >> Cc: mailto:serviceability-dev at openjdk.java.net
> >> Subject: Re: RFR(s): 8243451: nsk.share.jdi.Debugee.isJFR_active() is
> >> incorrect and corresponsing logic seems to be broken
> >> Hi Fairoz,
> >> What I think you need to do is something like this:
> >> if (className.equals("java.lang.Thread")) {
> >> return !isJfrInitialized();
> >> }
> >> ...
> >> private static boolean isJfrInitialized() {
> >> try {
> >> Class<?> clazz =
> >> Class.forName("jdk.jfr.FlightRecorder");
> >> Method method =
> >> clazz.getDeclaredMethod("isInitialized",
> >> new Class[0]);
> >> return (boolean) method.invoke(null, new Object[0]);
> >> } catch (Exception e) {
> >> return false;
> >> }
> >> }
> >> Erik
> >> On 2020-06-01 12:30, Fairoz Matte wrote:
> >> Hi Erik,
> >> Thanks for your quick response,
> >> Below is the updated webrev to handle if jfr module is not present
> >> http://cr.openjdk.java.net/~fmatte/8243451/webrev.01/
> >> Thanks,
> >> Fairoz
> >> -----Original Message-----
> >> From: Erik Gahlin
> >> Sent: Monday, June 1, 2020 2:31 PM
> >> To: Fairoz Matte mailto:fairoz.matte at oracle.com
> >> Cc: mailto:serviceability-dev at openjdk.java.net
> >> Subject: Re: RFR(s): 8243451: nsk.share.jdi.Debugee.isJFR_active() is
> >> incorrect and corresponsing logic seems to be broken
> >> Hi Fairoz,
> >> If the test needs to run with builds where the JFR module is not
> >> present(?), you need to do the check using reflection.
> >> If not, looks good.
> >> Erik
> >> On 1 Jun 2020, at 10:27, Fairoz Matte
> >> mailto:fairoz.matte at oracle.com wrote:
> >> Hi,
> >> Please review this small test infra change to identify at runtime
> >> the JFR is active or not.
> >> JBS - https://bugs.openjdk.java.net/browse/JDK-8243451
> >> Webrev - http://cr.openjdk.java.net/~fmatte/8243451/webrev.00/
> >> Thanks,
> >> Fairoz
> >>
> >
More information about the serviceability-dev
mailing list