RFR(s): 8243451: nsk.share.jdi.Debugee.isJFR_active() is incorrect and corresponsing logic seems to be broken
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Jun 10 07:28:49 UTC 2020
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