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 06:11:36 UTC 2020
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/jtreg/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