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

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