RFR(s) 4858370: JDWP: Memory Leak: GlobalRefs never deleted when processing invokeMethod command
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Tue Mar 22 10:05:27 UTC 2016
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 at 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
>>>
More information about the serviceability-dev
mailing list