Discussion:
Request for Review (XS): 7143760 Memory leak in GarbageCollectionNotifications
(too old to reply)
Frederic Parain
2012-02-10 09:27:30 UTC
Permalink
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
Dmitry Samersoff
2012-02-10 09:59:33 UTC
Permalink
Frederic,

GCNotificationRequest *request = getRequest();

request variable also leaks memory because it will never be deleted on
CHECK return path. Could you fix it too?

-Dmitry
Post by Frederic Parain
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.
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).
http://cr.openjdk.java.net/~fparain/7143760/webrev.00/
Thanks,
Fred
--
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...
David Holmes
2012-02-10 10:27:49 UTC
Permalink
Post by Dmitry Samersoff
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
Post by Dmitry Samersoff
-Dmitry
Post by Frederic Parain
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.
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).
http://cr.openjdk.java.net/~fparain/7143760/webrev.00/
Thanks,
Fred
Frederic Parain
2012-02-10 10:46:40 UTC
Permalink
I agree that the CHECK path is not memory clean.
I'm fixing it, I'll be back with better fix.

Thanks,

Fred
Post by Dmitry Samersoff
Frederic,
GCNotificationRequest *request = getRequest();
request variable also leaks memory because it will never be deleted on
CHECK return path. Could you fix it too?
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;
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
Post by Dmitry Samersoff
-Dmitry
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.
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).
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
Frederic Parain
2012-02-10 13:04:14 UTC
Permalink
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
Post by Dmitry Samersoff
Frederic,
GCNotificationRequest *request = getRequest();
request variable also leaks memory because it will never be deleted on
CHECK return path. Could you fix it too?
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;
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
Post by Dmitry Samersoff
-Dmitry
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.
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).
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
David Holmes
2012-02-13 02:56:20 UTC
Permalink
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
-----
Post by Frederic Parain
- 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
Post by Dmitry Samersoff
Frederic,
GCNotificationRequest *request = getRequest();
request variable also leaks memory because it will never be deleted on
CHECK return path. Could you fix it too?
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;
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
Post by Dmitry Samersoff
-Dmitry
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.
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).
http://cr.openjdk.java.net/~fparain/7143760/webrev.00/
Thanks,
Fred
Krystal Mok
2012-02-13 05:58:16 UTC
Permalink
Hi all,

Tto follow up with what David mentioned:

If sendNotification generates an exception then the serviceThread will
Post by David Holmes
terminate. Is that the desired behaviour? Other event processing can't
terminate the service thread.
I did a simple test to see what would happen in 7u2 when someone throws an
uncaught exception on the Service Thread [1]. And in this test it did make
the service thread terminate, because sendNotification() does:

try {
li.listener.handleNotification(notification, li.handback);
} catch (Exception e) {
e.printStackTrace();
throw new InternalError("Error in invoking listener");
}

And that InternalError gets uncaught, which leads to the termination of the
thread. So regardless of what the VM does, it seems like the current
behavior of the Service Thread is to terminate whenever a handler throws an
exception. Don't know if that's the desired behavior, though.

- Kris

[1]: https://gist.github.com/1814004
Post by David Holmes
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
-----
Post by Frederic Parain
- 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/<http://cr.openjdk.java.net/~fparain/7143760/webrev.01/>
Thanks,
Fred
Post by Dmitry Samersoff
Frederic,
GCNotificationRequest *request = getRequest();
request variable also leaks memory because it will never be deleted on
CHECK return path. Could you fix it too?
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;
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
Post by Dmitry Samersoff
-Dmitry
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.
http://bugs.sun.com/**bugdatabase/view_bug.do?bug_**id=7143760<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).
http://cr.openjdk.java.net/~**fparain/7143760/webrev.00/<http://cr.openjdk.java.net/~fparain/7143760/webrev.00/>
Thanks,
Fred
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20120213/30b5c99e/attachment.html
David Holmes
2012-02-13 06:08:50 UTC
Permalink
Post by Krystal Mok
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 did a simple test to see what would happen in 7u2 when someone throws
an uncaught exception on the Service Thread [1]. And in this test it did
try {
li.listener.handleNotification(notification, li.handback);
} catch (Exception e) {
e.printStackTrace();
throw new InternalError("Error in invoking listener");
}
To clarify what I was saying, the actual service thread logic does:

if (has_jvmti_events) {
jvmti_event.post();
}

if (sensors_changed) {
LowMemoryDetector::process_sensor_changes(jt);
}

if(has_gc_notification_event) {
GCNotifier::sendNotification(CHECK);
}

So only the GC notification events can trigger its termination due to
the exception. If this includes propagating exceptions from listener
code then this behaviour definitely seems incorrect to me.

David
-----
Post by Krystal Mok
And that InternalError gets uncaught, which leads to the termination of
the thread. So regardless of what the VM does, it seems like the current
behavior of the Service Thread is to terminate whenever a handler throws
an exception. Don't know if that's the desired behavior, though.
- Kris
[1]: https://gist.github.com/1814004
On Mon, Feb 13, 2012 at 10:56 AM, David Holmes <david.holmes at oracle.com
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
-----
- 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/
<http://cr.openjdk.java.net/~fparain/7143760/webrev.01/>
Thanks,
Fred
Frederic,
GCNotificationRequest *request = getRequest();
request variable also leaks memory because it will never
be deleted on
CHECK return path. Could you fix it too?
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;
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
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.
http://bugs.sun.com/__bugdatabase/view_bug.do?bug___id=7143760
<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).
http://cr.openjdk.java.net/~__fparain/7143760/webrev.00/
<http://cr.openjdk.java.net/~fparain/7143760/webrev.00/>
Thanks,
Fred
Frederic Parain
2012-02-13 09:17:01 UTC
Permalink
Post by Mikael Gerdin
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.
Post by Mikael Gerdin
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.
Post by Mikael Gerdin
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
Post by Mikael Gerdin
Post by Frederic Parain
- 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
Post by Dmitry Samersoff
Frederic,
GCNotificationRequest *request = getRequest();
request variable also leaks memory because it will never be deleted on
CHECK return path. Could you fix it too?
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;
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
Post by Dmitry Samersoff
-Dmitry
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.
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).
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
Frederic Parain
2012-02-13 10:11:26 UTC
Permalink
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
Post by Mikael Gerdin
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
-----
Post by Frederic Parain
- 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
Post by Dmitry Samersoff
Frederic,
GCNotificationRequest *request = getRequest();
request variable also leaks memory because it will never be deleted on
CHECK return path. Could you fix it too?
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;
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
Post by Dmitry Samersoff
-Dmitry
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.
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).
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
David Holmes
2012-02-14 06:58:43 UTC
Permalink
Hi Fred,
Post by Frederic Parain
http://cr.openjdk.java.net/~fparain/7143760/webrev.02/
- 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.
My only concern with this is that I don't see anything in the
javax.management docs describing the threading model for notifications
and listeners, or for how exceptions from listeners are processed.
Developers may assume that uncaught exception processing will occur if
their listeners encounter exceptions, whereas the implementation
"catches" all exceptions and ignores them. I guess this is more a JMX
spec issue.

So thumbs up from me.

David
-----
Post by Frederic Parain
Thanks,
Fred
Post by Mikael Gerdin
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
-----
Post by Frederic Parain
- 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
Post by Dmitry Samersoff
Frederic,
GCNotificationRequest *request = getRequest();
request variable also leaks memory because it will never be deleted on
CHECK return path. Could you fix it too?
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;
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
Post by Dmitry Samersoff
-Dmitry
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.
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).
http://cr.openjdk.java.net/~fparain/7143760/webrev.00/
Thanks,
Fred
Daniel D. Daugherty
2012-02-14 14:26:05 UTC
Permalink
Post by Frederic Parain
http://cr.openjdk.java.net/~fparain/7143760/webrev.02/
Please update the copyright years to 2012.

src/share/vm/services/gcNotifier.hpp
No comment.

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)

Nice refactoring.

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...

Thumbs up.

Dan
Post by Frederic Parain
- 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
Post by Mikael Gerdin
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
-----
Post by Frederic Parain
- 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
Post by Dmitry Samersoff
Frederic,
GCNotificationRequest *request = getRequest();
request variable also leaks memory because it will never be deleted on
CHECK return path. Could you fix it too?
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;
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
Post by Dmitry Samersoff
-Dmitry
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.
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).
http://cr.openjdk.java.net/~fparain/7143760/webrev.00/
Thanks,
Fred
Frederic Parain
2012-02-14 14:37:14 UTC
Permalink
Post by Daniel D. Daugherty
Post by Frederic Parain
http://cr.openjdk.java.net/~fparain/7143760/webrev.02/
Please update the copyright years to 2012.
Will do.
Post by Daniel D. Daugherty
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.
Post by Daniel D. Daugherty
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
Post by Daniel D. Daugherty
Post by Frederic Parain
- 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
Post by Mikael Gerdin
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
-----
Post by Frederic Parain
- 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
Post by Dmitry Samersoff
Frederic,
GCNotificationRequest *request = getRequest();
request variable also leaks memory because it will never be deleted on
CHECK return path. Could you fix it too?
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;
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
Post by Dmitry Samersoff
-Dmitry
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.
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).
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
keith mcguigan
2012-02-14 14:31:22 UTC
Permalink
Looks good to me.

--
- Keith
Post by Frederic Parain
http://cr.openjdk.java.net/~fparain/7143760/webrev.02/
- 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
Post by Mikael Gerdin
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
-----
Post by Frederic Parain
- 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
Post by Dmitry Samersoff
Frederic,
GCNotificationRequest *request = getRequest();
request variable also leaks memory because it will never be deleted on
CHECK return path. Could you fix it too?
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;
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
Post by Dmitry Samersoff
-Dmitry
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.
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).
http://cr.openjdk.java.net/~fparain/7143760/webrev.00/
Thanks,
Fred
Mikael Gerdin
2012-02-10 11:29:12 UTC
Permalink
Hi Fred,

I ran the repro on a build with your fix applied and it appears that the leak
is fixed. Thanks for finding it so quick :)

/Mikael
Post by Frederic Parain
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.
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).
http://cr.openjdk.java.net/~fparain/7143760/webrev.00/
Thanks,
Fred
Loading...