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.ah...@oracle.com>; serviceability-dev@openjdk.java.net Cc: Poonam Parhar <poonam.ba...@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
