[10] RFR for JDK-8169961: Memory leak after debugging session
Daniel D. Daugherty
daniel.daugherty at oracle.com
Thu Jul 20 15:01:21 UTC 2017
On 7/20/17 2:41 AM, Shafi Ahmad wrote:
> Hi Daniel,
>
>> -----Original Message-----
>> From: Langer, Christoph [mailto:christoph.langer at sap.com]
>> Sent: Monday, July 17, 2017 6:44 PM
>> To: Shafi Ahmad <shafi.s.ahmad at oracle.com>; Daniel Daugherty
>> <daniel.daugherty at oracle.com>
>> Cc: serviceability-dev at openjdk.java.net
>> Subject: RE: [10] RFR for JDK-8169961: Memory leak after debugging session
>>
>> Hi Shafi, Daniel,
>>
>> In my initial review I oversaw the usage of vm inside the constructor. The
>> new webrev looks even better. I guess the main fix is the termination of the
>> thread anyway.
>>
>> Best regards
>> Christoph
>>
>>> -----Original Message-----
>>> From: serviceability-dev [mailto:serviceability-dev-
>>> bounces at openjdk.java.net] On Behalf Of Shafi Ahmad
>>> Sent: Montag, 17. Juli 2017 14:25
>>> To: Daniel Daugherty <daniel.daugherty at oracle.com>; serviceability-
>>> dev at openjdk.java.net
>>> Subject: RE: [10] RFR for JDK-8169961: Memory leak after debugging
>>> session
>>>
>>> Hi Daniel,
>>>
>>> Thank you for the review.
>>>
>>>> -----Original Message-----
>>>> From: Daniel D. Daugherty
>>>> Sent: Friday, July 14, 2017 6:46 AM
>>>> To: Shafi Ahmad <shafi.s.ahmad at oracle.com>; serviceability-
>>>> dev at openjdk.java.net
>>>> Subject: Re: [10] RFR for JDK-8169961: Memory leak after debugging
>>> session
>>>> On 7/13/17 10:19 AM, Shafi Ahmad wrote:
>>>>> Hi Daniel,
>>>>>
>>>>> Thank you for the review.
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Daniel D. Daugherty
>>>>>> Sent: Thursday, July 13, 2017 7:40 PM
>>>>>> To: Shafi Ahmad <shafi.s.ahmad at oracle.com>; serviceability-
>>>>>> dev at openjdk.java.net
>>>>>> Subject: Re: [10] RFR for JDK-8169961: Memory leak after
>>>>>> debugging session
>>>>>>
>>>>>> On 7/13/17 2:45 AM, Shafi Ahmad wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review the code change for the fix of bug 'JDK-8169961:
>>>>>>> Memory
>>>>>> leak after debugging session'
>>>>>>> Summary:
>>>>>>> 1. It seems that the thread created for
>>>>>> com.sun.tools.jdi.TargetVM.EventController is never stopped and
>>> keeps
>>>>>> a hard reference to the VirtualMachineImpl. This leads to
>>>>>> VirtualMachineImpl leak after debug session is finished.
>>>>>> EventController is private class and member field vm of type
>>>>>> VirtualMachineImpl, holding the hard reference to the
>>>>>> VirtualMachineImpl. I am not seeing the usage of filed vm so we
>>>>>> can
>>>> remove it safely.
>>>>>>> 2. Added eventController.release(); before 'Target VM interface
>>>>>>> thread
>>>>>> exiting'
>>>>>>> 3. TargetVM gets an EventController which is a daemon thread,
>>>>>>> but don't
>>>>>> see the thread having a way of stopping so added code to exit as
>>>>>> soon as TargetVM thread stops listening.
>>>>>>> jdk10 bug: https://bugs.openjdk.java.net/browse/JDK-8169961
>>>>>>> webrev link:
>>>> http://cr.openjdk.java.net/~shshahma/8169961/webrev.00/
>>>>>> src/jdk.jdi/share/classes/com/sun/tools/jdi/TargetVM.java
>>>>>> L330: VirtualMachineImpl vm;
>>>>>> L335: this.vm = vm;
>>>>>> This comment caught my eye:
>>>>>> > I am not seeing the usage of filed vm so we can remove it
>> safely.
>>>>>> So you're deleting the 'vm' instance variable, but:
>>>>>>
>>>>>> L363: JDWP.VirtualMachine.HoldEvents.process(vm);
>>>>>> L365: JDWP.VirtualMachine.ReleaseEvents.process(vm);
>>>>>>
>>>>>> would use it if it was still there...
>>>>>>
>>>>>> Where did the run() method find a 'vm' variable? I'm guessing
>>>>>> the one in the TargetVM outer class:
>>>>>>
>>>>>> L46: private VirtualMachineImpl vm;
>>>>>>
>>>>>> Okay so we don't have to pass (and save a copy of
>>>> VirtualMachineImpl).
>>>>>> So this constructor:
>>>>>>
>>>>>> L333: EventController(VirtualMachineImpl vm) {
>>>>>> No longer uses the 'vm' parameter at all and you
>>>>>> can remove the parameter and update the caller.
>>>>> 332 private class EventController extends Thread {
>>>>> 333 int controlRequest = 0;
>>>>> 334
>>>>> 335 EventController(VirtualMachineImpl vm) {
>>>>> 336 super(vm.threadGroupForJDI(), "JDI Event Control Thread");
>>>>> 337 setDaemon(true);
>>>>> 338 setPriority((MAX_PRIORITY + NORM_PRIORITY)/2);
>>>>> 339 super.start();
>>>>> 340 }
>>>>>
>>>>> We can't remove formal parameter 'vm' as this is referenced at line#
>> 336.
>>>> So the reference to 'vm' on L336 won't resolve to the one in the
>>>> TargetVM outer class:
>>>>
>>>> L46: private VirtualMachineImpl vm;
>>>>
>>>> like L363 and L365 do...
>>> Yes, you are right as this is an inner class constructor.
>>>
>>> Please find updated webrev.
>>> http://cr.openjdk.java.net/~shshahma/8169961/webrev.01/
>>>
>>> Regards,
>>> Shafi
>>>
>>>
>>>> Dan
>>>>
>>>>
>>>>>>> Testing: run jprt and I provided the FBP and it works for them.
>>>>>> JPRT doesn't execute any JDI related tests. You need to run the
>>>>>> NSK JDI test suite and the com/sun/jdi tests in the JDK.
>>>>> I will run NSK JDI test and update this thread with the results.
> I have run the nsk.jdi tests and all are fine.
>
> TEST 429/1062 429/1062 nsk/jdi/ReferenceType/visibleMethods/visibmethod003 PASS
> TEST 646/1062 646/1062 nsk/jdi/ClassUnloadRequest/addClassFilter/filter001 PASS
> TEST 648/1062 648/1062 nsk/jdi/ClassUnloadRequest/addClassExclusionFilter/exclfilter001 PASS
> HARNESS ENDED: Thu Jul 20 01:02:31 PDT 2017
>
> TOTAL TESTS IN RUN: 1062
> TEST PASS: 1062; 100% rate
> TEST FAIL: 0; 0% rate
> TEST UNDEFINED: 0; 0% rate
> TEST INCOMPLETE: 0; 0% rate
> TESTS NOT RUN: 0
>
> TOTAL TEST IN TESTLIST: 1142
>
> Finished building target 'test-hotspot-closed-tonga-only' in configuration 'linux-x64'
> shshahma at slc12kkg:/scratch/shshahma/Java/jdk10-hs$ cd jdk/
> shshahma at slc12kkg:/scratch/shshahma/Java/jdk10-hs/jdk$ st
> M src/jdk.jdi/share/classes/com/sun/tools/jdi/TargetVM.java
Thanks for doing the extra testing.
Dan
>
>
> Regards,
> Shafi
>
>>>>> Regards,
>>>>> Shafi
>>>>>
>>>>>> Dan
>>>>>>
>>>>>>> Regards,
>>>>>>> Shafi
More information about the serviceability-dev
mailing list