Hi Daniel, > -----Original Message----- > From: Langer, Christoph [mailto:christoph.lan...@sap.com] > Sent: Monday, July 17, 2017 6:44 PM > To: Shafi Ahmad <shafi.s.ah...@oracle.com>; Daniel Daugherty > <daniel.daughe...@oracle.com> > Cc: serviceability-dev@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- > > boun...@openjdk.java.net] On Behalf Of Shafi Ahmad > > Sent: Montag, 17. Juli 2017 14:25 > > To: Daniel Daugherty <daniel.daughe...@oracle.com>; serviceability- > > d...@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.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.
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@slc12kkg:/scratch/shshahma/Java/jdk10-hs$ cd jdk/ shshahma@slc12kkg:/scratch/shshahma/Java/jdk10-hs/jdk$ st M src/jdk.jdi/share/classes/com/sun/tools/jdi/TargetVM.java Regards, Shafi > > > > Regards, > > > > Shafi > > > > > > > >> Dan > > > >> > > > >>> Regards, > > > >>> Shafi > > >
