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:35:53 UTC 2020
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.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