Request for Review (XS): 7143760 Memory leak in GarbageCollectionNotifications

[Frederic Parain]

On 02/13/12 03:56 AM, David Holmes wrote:
> Hi Fred,
>
> 182 class NotificationMark : public StackObj {
>
> Really we should have a general purpose utility class that can serve in
> this role. This is the second time in a couple of weeks that the need to
> deal with cleanup with CHECK has been uncovered.
>
> Not saying you necessarily need to do it for this CR.

I agree, but clearly out the scope of this CR.

> 183 // This class is used in GCNitifer::sendNotification to ensure
>
> Typo: nitifer :)
>
> 203 Handle objGcInfo =
> createGcInfo(request->gcManager,request->gcStatInfo, CHECK);
>
> 212 instanceOop gc_mbean =
> request->gcManager->get_memory_manager_instance(CHECK);
>
> CHECK should only be added to functions that can cause exceptions to
> become pending.

Oops, I'll revert it.

> That all said I'm not sure that this fix hasn't gone the wrong way. If
> sendNotification generates an exception then the serviceThread will
> terminate. Is that the desired behaviour? Other event processing can't
> terminate the service thread.

I forgot to check that. The initial notification implementation was
using its own thread which was protected against pending exceptions.
The service thread is not. I'll add a wrapper around the
sendNotification() method to catch all exceptions.

Thanks for this useful review.

Fred

>
> On 10/02/2012 11:04 PM, Frederic Parain wrote:
>> Here's a new webrev addressing the following issues:
>> - the missing HandleMark
>> - the clean up of the GCNotificationRequest instance
>> - removal of the pending exception testing, now
>> exception will be propagated as soon as a method
>> returns with a pending exception
>>
>> http://cr.openjdk.java.net/~fparain/7143760/webrev.01/
>>
>> Thanks,
>>
>> Fred
>>
>> On 2/10/12 11:27 AM, David Holmes wrote:
>>> On 10/02/2012 7:59 PM, Dmitry Samersoff wrote:
>>>> Frederic,
>>>>
>>>> GCNotificationRequest *request = getRequest();
>>>>
>>>> request variable also leaks memory because it will never be deleted on
>>>> CHECK return path. Could you fix it too?
>>>
>>> Further:
>>>
>>> 211 JavaCalls::call_virtual(&result,
>>> 212 gc_mbean_klass,
>>> 213 vmSymbols::createGCNotification_name(),
>>> 214 vmSymbols::createGCNotification_signature(),
>>> 215 &args,
>>> 216 CHECK);
>>> 217 if (HAS_PENDING_EXCEPTION) {
>>> 218 CLEAR_PENDING_EXCEPTION;
>>> 219 }
>>> 220
>>> 221 delete request;
>>>
>>> The CHECK at @216 will cause a return if there is an exception pending
>>> so 217-219 is dead code. This also indicates some confusion about what
>>> exceptions this method can leave pending. Or it may be that the CHECK at
>>> #216 was meant to be just THREAD. ??
>>>
>>> (Strange this is the second example I've seen of this today!)
>>>
>>> David
>>>
>>>>
>>>> -Dmitry
>>>>
>>>>
>>>> On 2012-02-10 13:27, Frederic Parain wrote:
>>>>> Here's a small fix (one line) for CR 7143760 Memory leak in
>>>>> GarbageCollectionNotifications
>>>>>
>>>>> There's a missing HandleMark at the beginning of the
>>>>> GCNotifier::sendNotificatin() method. Without this HandleMark, all
>>>>> handles used when creating GC notifications are kept alive causing a
>>>>> double leak: in the Java heap and in the thread local handle area of
>>>>> the
>>>>> service thread.
>>>>>
>>>>> Here's the CR:
>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7143760
>>>>> (Warning, the changeset referenced in the CR is not the
>>>>> one containing the original bug).
>>>>>
>>>>> Here's the webrev:
>>>>> http://cr.openjdk.java.net/~fparain/7143760/webrev.00/
>>>>>
>>>>> Thanks,
>>>>>
>>>>> Fred
>>>>>
>>>>
>>>>
>>

-- 
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: Frederic.Parain at Oracle.com