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.ah...@oracle.com>; serviceability- > d...@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. > > > 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. Regards, Shafi > > Dan > > > > > Regards, > > Shafi >
