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

Leonid Mesnik leonid.mesnik at oracle.com
Wed Jun 10 16:48:41 UTC 2020 

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