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

[Frederic Parain]

On 02/14/12 03:26 PM, Daniel D. Daugherty wrote:
> On 2/13/12 3:11 AM, Frederic Parain wrote:
>> Here's the new webrev:
>> http://cr.openjdk.java.net/~fparain/7143760/webrev.02/
>
> Please update the copyright years to 2012.

Will do.

> src/share/vm/services/gcNotifier.cpp
> nit line 185: space missing in 'if(' (yes, I know you didn't break that)
> nit line 212: missing space after ',' (you fixed one but not the other)

Will do.

> In the old code, CLEAR_PENDING_EXCEPTION would not be called if
> getRequest() returned NULL. In the new code, it will. I don't
> think that will make much of a difference, but I'm not an expert
> in this code...

It won't make a difference. The service thread should not have
any pending exception when it calls GCNotifier::sendNotification(),
so the "if (HAS_PENDING_EXCEPTION)" test should return false when
getRequest() returns NULL.

Thanks,

Fred

>> The changes are:
>> - fix typo in comments
>> - roll-back wrong THREAD->CHECK modifications
>> - add a wrapper catching exceptions around the method
>> sending notifications in order to prevent premature
>> termination of the service thread.
>>
>> Thanks,
>>
>> Fred
>>
>> 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.
>>>
>>> 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.
>>>
>>> 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.
>>>
>>> David
>>> -----
>>>
>>>
>>> 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