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

[keith mcguigan]

Looks good to me.

--
- Keith

Frederic Parain wrote:
> Here's the new webrev:
> http://cr.openjdk.java.net/~fparain/7143760/webrev.02/
> 
> 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
>>>>>>
>>>>>
>>>>>
>>>
>