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