RFR(s): 8243451: nsk.share.jdi.Debugee.isJFR_active() is incorrect and corresponsing logic seems to be broken

[serguei.spitsyn at oracle.com]

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
>   
>   
>