Thanks Serguei, Leonid for the reviews. Thanks, Fairoz
> -----Original Message----- > From: Leonid Mesnik > Sent: Wednesday, June 10, 2020 10:19 PM > To: Fairoz Matte <[email protected]> > Cc: Serguei Spitsyn <[email protected]>; Erik Gahlin > <[email protected]>; [email protected] > Subject: Re: RFR(s): 8243451: nsk.share.jdi.Debugee.isJFR_active() is > incorrect > and corresponsing logic seems to be broken > > Looks good, no other webrev is needed. > > Leonid > > On 6/10/20 12:28 AM, [email protected] wrote: > > > > On 6/9/20 23:35, Fairoz Matte wrote: > >> Hi Serguei, > >> > >> Thanks for the clarification. > >> I will work on to move isJFRActive () method from the > >> TestDebuggerType2 to HeapWalkingDebugger > > > > Probably, there is no need in another webrev if you move it. > > But you did not get a final thumbs up from Leonid yet. > > > > Thanks, > > Serguei > > > >> Thanks, > >> Fairoz > >> > >>> -----Original Message----- > >>> From: Serguei Spitsyn > >>> Sent: Wednesday, June 10, 2020 11:42 AM > >>> To: Fairoz Matte <[email protected]>; Leonid Mesnik > >>> <[email protected]>; Erik Gahlin <[email protected]> > >>> Cc: [email protected] > >>> 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 <[email protected]>; Fairoz Matte > >>>>> <[email protected]>; Erik Gahlin <[email protected]> > >>>>> Cc: [email protected] > >>>>> 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, [email protected] 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 <[email protected]>; Erik Gahlin > >>>>>>> <[email protected]> > >>>>>>> Cc: [email protected] > >>>>>>> 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:[email protected] 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:[email protected]; Erik Gahlin > >>>>>>> mailto:[email protected] > >>>>>>> Cc: mailto:[email protected] > >>>>>>> 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:[email protected] 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:[email protected] > >>>>>>> Cc: mailto:[email protected] > >>>>>>> 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:[email protected] > >>>>>>> Cc: mailto:[email protected] > >>>>>>> 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:[email protected] 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 > >>>>>>> > >
