[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(s) 4858370: JDWP: Memory Leak: GlobalRefs never deleted when processing invokeMethod command
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2016-03-22 10:05:27
Message-ID: 56F118E7.5030508 () oracle ! com
[Download RAW message or body]

Hi Severin,

It looks good to me.
Thank you for the changes!

I'll wait some time for any other review before the push.


Thanks,
Serguei



On 3/22/16 02:54, Severin Gehwolf wrote:
> Hi Serguei,
> 
> On Mon, 2016-03-21 at 14:38 -0700, serguei.spitsyn@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
> > > 


[prev in list] [next in list] [prev in thread] [next in thread]