[10] RFR for JDK-8170299: Debugger does not stop inside the low memory notifications code

[Shafi Ahmad]

Hi Mandy,

 

Thank you for the review comments.

 

>> I suggest to file a RFE to consider adding hasListeners method as a separate issue. 

I will file a RFE for this.

 

>> This creates a new thread for each listener to handle each notification which is overkill.   You can create one system daemon thread to handle all notifications for all listeners.

Please correct me if I don't understand correctly, instead of using thread as an Executor I should use system daemon thread as an Executor.

 

>> For this fix, you could simply update NotificationEmitterSupport to create a system daemon thread to handle all notifications.  

I have a doubt if I will update the NotificationEmitterSupport then I am not sure how to pass an Executor. NotificationEmitterSupport doesn't takes an Executor.

 

Regards,

Shafi

 

 

From: mandy chung 
Sent: Tuesday, September 12, 2017 2:19 AM
To: Shafi Ahmad <shafi.s.ahmad at oracle.com>; serviceability-dev at openjdk.java.net
Cc: Poonam Parhar <poonam.bajaj at oracle.com>
Subject: Re: [10] RFR for JDK-8170299: Debugger does not stop inside the low memory notifications code

 

 

On 9/4/17 3:11 AM, Shafi Ahmad wrote:





The method hasListeners() is referenced  inside MemoryImpl.java and defined in  NotificationEmitterSupport.

This method is not present in  NotificationBroadcasterSupport so I added it to NotificationBroadcasterSupport.

 

Jdk10 bug: https://bugs.openjdk.java.net/browse/JDK-8170299

Webrev link: HYPERLINK "http://cr.openjdk.java.net/%7Eshshahma/8170299/webrev.01/"http://cr.openjdk.java.net/~shshahma/8170299/webrev.01/

 

This patch adds a new public method in NotificationBroadcasterSupport.  I think this fix wants to avoid adding this public method since this fix is intended to be backport.

MemoryImpl checks if there is any listener registered to avoid instantiating the notification object if no listener handles it.

Replacing the internal sun.management.NotificationEmitterSupport with NotificationBroadcasterSupport is a good change.  I suggest to file a RFE to consider adding hasListeners method as a separate issue.  

 171     static final class ThreadExecutor implements Executor {
 172         public void execute(Runnable r) { new Thread(r).start(); }
 173     } 
 

This creates a new thread for each listener to handle each notification which is overkill.   You can create one system daemon thread to handle all notifications for all listeners.  

For this fix, you could simply update NotificationEmitterSupport to create a system daemon thread to handle all notifications.  NotificationEmitterSupport::sendNotification should also be updated to ignore the exception (currently it throws an exception).  

Mandy

 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20170913/9d15838e/attachment-0001.html>