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

[mandy chung]

On 9/13/17 4:40 AM, Shafi Ahmad wrote:

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.

You can still use Executor and what I meant is to use one single thread 
for handling the low memory notification rather than one thread per 
notification.

See Executors::newSingleThreadExecutor(ThreadFactory factory) and the 
thread factory can create one InnocuousThreadFactory::newSystemThread 
and set it to daemon thread.

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

Modify NotificationEmitterSupport to have a static Executor initialized 
as described above.  I can send you the sample code to do that - just 
let me know.

Mandy

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:
    http://cr.openjdk.java.net/~shshahma/8170299/webrev.01/
    <http://cr.openjdk.java.net/%7Eshshahma/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