[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: [10] RFR for JDK-8170299: Debugger does not stop inside the low memory notifications code
From: mandy chung <mandy.chung () oracle ! com>
Date: 2017-09-13 16:41:19
Message-ID: 3f0135e8-5dbf-4c86-6a1d-5db97893c45c () oracle ! com
[Download RAW message or body]
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@oracle.com>;
> serviceability-dev@openjdk.java.net
> *Cc:* Poonam Parhar <poonam.bajaj@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
>
[Attachment #3 (text/html)]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<br>
<br>
<div class="moz-cite-prefix">On 9/13/17 4:40 AM, Shafi Ahmad wrote:<br>
</div>
<blockquote type="cite"
cite="mid:8ae3e612-5b05-42ce-a7ed-6d193ec637f0@default">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<meta name="Generator" content="Microsoft Word 15 (filtered
medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
@font-face
{font-family:Menlo;
panose-1:0 0 0 0 0 0 0 0 0 0;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:#0563C1;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:#954F72;
text-decoration:underline;}
p.MsoPlainText, li.MsoPlainText, div.MsoPlainText
{mso-style-priority:99;
mso-style-link:"Plain Text Char";
margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";
color:black;}
p.MsoListParagraph, li.MsoListParagraph, div.MsoListParagraph
{mso-style-priority:34;
margin-top:0in;
margin-right:0in;
margin-bottom:0in;
margin-left:.5in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0in;
mso-margin-bottom-alt:auto;
margin-left:0in;
font-size:12.0pt;
font-family:"Times New Roman",serif;
color:black;}
span.PlainTextChar
{mso-style-name:"Plain Text Char";
mso-style-priority:99;
mso-style-link:"Plain Text";
font-family:"Calibri",sans-serif;}
span.EmailStyle21
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle22
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;
color:black;}
span.new
{mso-style-name:new;}
span.EmailStyle26
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
<div class="WordSection1">
<p class="MsoNormal"><span style="color:windowtext">Hi \
Mandy,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:windowtext"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:windowtext">Thank you
for the review comments.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:windowtext"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:windowtext">>> </span><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">I suggest to file a RFE to consider
adding hasListeners method as a separate issue. <o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">I will file a RFE for this.<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"><o:p> </o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">>> 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.<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">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.<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"><o:p> </o:p></span></p>
</div>
</blockquote>
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.<br>
<br>
See Executors::newSingleThreadExecutor(ThreadFactory factory) and
the thread factory can create one
InnocuousThreadFactory::newSystemThread and set it to daemon thread.
<br>
<br>
<blockquote type="cite"
cite="mid:8ae3e612-5b05-42ce-a7ed-6d193ec637f0@default">
<div class="WordSection1">
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">>> For this fix, you could simply
update NotificationEmitterSupport to create a system daemon
thread to handle all notifications. <o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">I have a doubt if I will update the
NotificationEmitterSupport then I am not sure how to pass an
Executor. NotificationEmitterSupport doesn’t </span>takes
an Executor.<o:p></o:p></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"><o:p> </o:p></span></p>
</div>
</blockquote>
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.<br>
<br>
Mandy<br>
<blockquote type="cite"
cite="mid:8ae3e612-5b05-42ce-a7ed-6d193ec637f0@default">
<div class="WordSection1">
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">Regards,<o:p></o:p></span></p>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">Shafi<o:p></o:p></span></p>
<p class="MsoNormal"><span style="color:windowtext"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="color:windowtext"><o:p> </o:p></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in
0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #E1E1E1
1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span \
style="color:windowtext">From:</span></b><span style="color:windowtext"> mandy chung \
<br> <b>Sent:</b> Tuesday, September 12, 2017 2:19 AM<br>
<b>To:</b> Shafi Ahmad
<a class="moz-txt-link-rfc2396E" \
href="mailto:shafi.s.ahmad@oracle.com"><shafi.s.ahmad@oracle.com></a>;
<a class="moz-txt-link-abbreviated" \
href="mailto:serviceability-dev@openjdk.java.net">serviceability-dev@openjdk.java.net</a><br>
<b>Cc:</b> Poonam Parhar
<a class="moz-txt-link-rfc2396E" \
href="mailto:poonam.bajaj@oracle.com"><poonam.bajaj@oracle.com></a><br> \
<b>Subject:</b> Re: [10] RFR for JDK-8170299: Debugger
does not stop inside the low memory notifications \
code<o:p></o:p></span></p> </div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span
style="font-size:12.0pt"><o:p> </o:p></span></p>
<div>
<p class="MsoNormal">On 9/4/17 3:11 AM, Shafi Ahmad wrote:<o:p></o:p></p>
</div>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><br>
<br>
<o:p></o:p></p>
<p class="MsoNormal">The method hasListeners() is referenced
inside MemoryImpl.java and defined in
NotificationEmitterSupport.<o:p></o:p></p>
<p class="MsoNormal">This method is not present in
NotificationBroadcasterSupport so I added it to
NotificationBroadcasterSupport.<o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal">Jdk10 bug: <a
href="https://bugs.openjdk.java.net/browse/JDK-8170299"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8170299</a><o:p></o:p></p>
<p class="MsoNormal">Webrev link: <a
href="http://cr.openjdk.java.net/%7Eshshahma/8170299/webrev.01/"
moz-do-not-send="true">http://cr.openjdk.java.net/~shshahma/8170299/webrev.01/</a><o:p></o:p></p>
<p class="MsoNormal"> <o:p></o:p></p>
</blockquote>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">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.<br>
<br>
MemoryImpl checks if there is any listener registered to
avoid instantiating the notification object if no listener
handles it.<br>
<br>
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. <o:p></o:p></span></p>
<pre><span class="new"> 171 static final class ThreadExecutor \
implements Executor {</span><o:p></o:p></pre>
<pre><span class="new"> 172 public void execute(Runnable r) { new \
Thread(r).start(); }</span><o:p></o:p></pre> <pre><span class="new"> 173 } \
<o:p></o:p></span></pre> <pre><o:p> </o:p></pre>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif">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. <br>
<br>
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). <o:p></o:p></span></p>
<pre style="background:white"><span \
style="font-size:9.0pt;font-family:"Menlo",serif">Mandy<o:p></o:p></span></pre>
<p class="MsoNormal"><span
style="font-size:12.0pt;font-family:"Times New
Roman",serif"><o:p> </o:p></span></p>
</div>
</div>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic