Hi Serguei, On Mon, 2016-03-21 at 14:38 -0700, serguei.spit...@oracle.com wrote: > Hi Severin, > > Thank you for taking care about this issue! > > The fix looks pretty good.
Thanks for the quick review! > A couple of minor comments though. > > src/jdk.jdwp.agent/share/native/libjdwp/invoker.c > 222 jint argIndex; > 223 jbyte argumentTag; > 224 jvalue *argument; > 225 void *cursor; > . . . > 234 argIndex = 0; > 235 argumentTag = firstArgumentTypeTag(request->methodSignature, > &cursor); > 236 argument = request->arguments; > > Could you make a small simplification and re-arrange the initialization of > the locals: > void *cursor; > jint argIndex = 0; > jvalue *argument = request->arguments; > jbyte argumentTag = firstArgumentTypeTag(request->methodSignature, > &cursor); > > I'd also suggest to get rid of the extra spaces at the lines: 227, 230, > 237, 240, 252. It will make the code style to be more consistent. Could > you, correct the comment at line 216 (invoker_invoke*() => invoker invoke*()): > 216 * invoke request was carried out. See fillInvokeRequest() and > invoker_invoke*() > > > test/com/sun/jdi/OomDebugTest.java Some lines are too long: 142, 162, 197, > 212, 228, 264, 267, 289 It would be nice to make them shorter. I can push > the fix once it has been reviewed. Thanks, Serguei Updated webrev with the changes above is here: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-4858370/webrev.02/ For convenience, I've uploaded the HG exported patch too: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-4858370/JDK-4858370-jdk9-jdk.export.patch Cheers, Severin > On 3/21/16 10:47, Severin Gehwolf wrote: > > Hi, > > > > Could somebody please review this fix for bug 4858370? > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-4858370 > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-4858370/webrev.01/ > > > > Testing done: jdk_jdi test group passes. Added regression test. > > > > There is a memory leak in the JDWP implementation where method > > parameters, method return values and the "this" object reference for > > constructor invocations are kept in memory via a global reference and > > get, thus, never GC'ed. > > > > The proposed fix deletes global references again in > > invoke_completeInvokeRequest(). The references got previously created > > in fillInvokeRequest() and invoker_invoke*() implementations. > > > > Thoughts? > > > > Note that I'd need somebody to sponsor the push to JDK 9 tree for me > > (once approved). Thanks. > > > > Cheers, > > Severin > >
