Hi Serguei, Thanks for the clarification. I will work on to move isJFRActive () method from the TestDebuggerType2 to HeapWalkingDebugger
Thanks, Fairoz > -----Original Message----- > From: Serguei Spitsyn > Sent: Wednesday, June 10, 2020 11:42 AM > To: Fairoz Matte <fairoz.ma...@oracle.com>; Leonid Mesnik > <leonid.mes...@oracle.com>; Erik Gahlin <erik.gah...@oracle.com> > Cc: serviceability-dev@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, > > It is confusing there is methods with the same name isJFRActive on both > debuggee and debugger side. > Leonid is talking about the isJFRActive that belongs to the debugger. > He suggests to move this method from the TestDebuggerType2 to > HeapWalkingDebugger. > The reason is the HeapWalkingDebugger should have a knowledge about the > HeapWalkingDebuggee, not its super class TestDebuggerType2. > It looks like a good suggestion to me. > > Thanks, > Serguei > > > On 6/9/20 23:00, Fairoz Matte wrote: > > 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.spit...@oracle.com>; Fairoz Matte > >> <fairoz.ma...@oracle.com>; Erik Gahlin <erik.gah...@oracle.com> > >> Cc: serviceability-dev@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/jtr > >> eg/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.spit...@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.ma...@oracle.com>; Erik Gahlin > >>>> <erik.gah...@oracle.com> > >>>> Cc: serviceability-dev@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.spit...@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.ma...@oracle.com; Erik Gahlin > >>>> mailto:erik.gah...@oracle.com > >>>> Cc: mailto:serviceability-dev@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.spit...@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.ma...@oracle.com > >>>> Cc: mailto:serviceability-dev@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.ma...@oracle.com > >>>> Cc: mailto:serviceability-dev@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.ma...@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 > >>>> >
