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
Thu Jun 11 04:59:00 UTC 2020
Thanks Serguei, Leonid for the reviews.
Thanks,
Fairoz
> -----Original Message-----
> From: Leonid Mesnik
> Sent: Wednesday, June 10, 2020 10:19 PM
> To: Fairoz Matte <fairoz.matte at oracle.com>
> Cc: Serguei Spitsyn <serguei.spitsyn at oracle.com>; Erik Gahlin
> <erik.gahlin at oracle.com>; 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
>
> Looks good, no other webrev is needed.
>
> Leonid
>
> On 6/10/20 12:28 AM, serguei.spitsyn at oracle.com 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 <fairoz.matte at oracle.com>; Leonid Mesnik
> >>> <leonid.mesnik 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,
> >>>
> >>> 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.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/
> >>>>> 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.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