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
