Hi Shafi,
I'm not at all sure about the approach you've taken with this ...
On 11/12/2017 8:31 PM, Shafi Ahmad wrote:
Hi,
Could you please review the fix for JDK-8170299: Debugger does not stop
inside the low memory notifications code.
With the current implementation ‘debugger does not stop inside the low
memory *notifications* code’
This is expected as the notification code is getting executed with the
service thread, which is a hidden thread.
Okay. I recall some discussion on this problem - the main question being
"why is the service thread executing user-defined Java code?". I'm
assuming now this is an implementation artifact, not part of the main
design of the management framework? But it is unclear. Attempting to
introduce a new thread to execute the code may be reasonable, but raises
whole range of questions about concurrency, synchronization and
possibly deadlock with this code!
shafi@shafi-ahmad:~/Java/jdk8/jdk8u-dev$ jdb -Xmx32m MemoryWarningSystem
Initializing jdb ...
> stop at MemoryWarningSystem$1:25
Deferring breakpoint MemoryWarningSystem$1:25.
It will be set after the class is loaded.
> run
run MemoryWarningSystem
Set uncaught java.lang.Throwable
Set deferred uncaught java.lang.Throwable
>
. . .
> quit
Breakpoint not hit.
With the attached webrev, jdb stop at breakpoint inside notification code.
shshahma@slc12kkg:/scratch/shshahma/Java/jdk-dev$ jdb -Xmx128m
MemoryWarningSystem
Initializing jdb ...
> stop at MemoryWarningSystem$1:25
Deferring breakpoint MemoryWarningSystem$1:25.
It will be set after the class is loaded.
> run
run MemoryWarningSystem
Set uncaught java.lang.Throwable
Set deferred uncaught java.lang.Throwable
>
VM Started: Set deferred breakpoint MemoryWarningSystem$1:25
Breakpoint hit: "thread=Debugger",
MemoryWarningSystem$1.memoryUsageLow(), line=25 bci=0
25 System.out.println("Memory usage low!!!");
Debugger[1] where
[1] MemoryWarningSystem$1.memoryUsageLow (MemoryWarningSystem.java:25)
[2] MemoryWarningSystem$2.handleNotification
(MemoryWarningSystem.java:48)
[3]
javax.management.NotificationBroadcasterSupport.handleNotification
(NotificationBroadcasterSupport.java:284)
[4] javax.management.NotificationBroadcasterSupport$SendNotifJob.run
(NotificationBroadcasterSupport.java:361)
[5] java.util.concurrent.ThreadPoolExecutor.runWorker
(ThreadPoolExecutor.java:1,135)
[6] java.util.concurrent.ThreadPoolExecutor$Worker.run
(ThreadPoolExecutor.java:635)
[7] java.lang.Thread.run (Thread.java:844)
[8] jdk.internal.misc.InnocuousThread.run (InnocuousThread.java:134)
Debugger[1] list
21
22 MemoryWarningSystem mws = new MemoryWarningSystem();
23 mws.addListener(new MemoryWarningSystem.Listener() {
24 public void memoryUsageLow(long usedMemory, long maxMemory) {
25 => System.out.println("Memory usage low!!!");
26 double percentageUsed = ((double) usedMemory) / maxMemory;
27 System.out.println("percentageUsed = " + percentageUsed);
28 MemoryWarningSystem.setPercentageUsageThreshold(0.8);
29 }
30 });
Debugger[1] threads
Group system:
(java.lang.ref.Reference$ReferenceHandler)0x362 Reference Handler running
(java.lang.ref.Finalizer$FinalizerThread)0x361 Finalizer
cond. waiting
(java.lang.Thread)0x360 Signal Dispatcher running
Group main:
(java.lang.Thread)0x1 main running
Group InnocuousThreadGroup:
(jdk.internal.misc.InnocuousThread)0x35f Common-Cleaner
cond. waiting
(jdk.internal.misc.InnocuousThread)0x4f1 Debugger
running (at breakpoint)
Debugger[1] step
> Memory usage low!!!
Step completed: "thread=Debugger",
MemoryWarningSystem$1.memoryUsageLow(), line=26 bci=8
26 double percentageUsed = ((double) usedMemory) / maxMemory;
Please see the newly created thread “
(jdk.internal.misc.InnocuousThread)0x4f1 Debugger
running (at breakpoint)”.
In the change, the class ‘MemoryImpl’ is derived from class
‘NotificationBroadcasterSupport’ instead of class
‘NotificationEmitterSupport’
That's quite a significant change and the implications of it are not at
all obvious. The sun.management.NotificationEmitterSupport class
utilises synchronization for thread-safety. The
NotificationBroadcasterSupport class utilises a CopyOnWriteArraylist and
will have quite different concurrency properties. Throw into the mix the
fact you've now added a new thread and we really have no clear idea how
this will all behave.
NotificationBroadcastSupport takes an Executor, in the constructor of
MemoryImpl we are calling super() with an executor, which is a visible
thread.
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.
You will need to file a CSR request first to add a public method to a
public class. But it's not all clear to me this is a method that should
be added to this API.
I see what you are trying to do, but the impact of these changes seem
quite far reaching and hard to determine.
Jdk10 bug: https://bugs.openjdk.java.net/browse/JDK-8170299
You should note that the bug is not publicly visible.
Thanks,
David
Webrev link: http://cr.openjdk.java.net/~shshahma/8170299/webrev.02/
Testing: jprt -testset core, rbt --test nsk.jvmti.testlist,nsk.jdi.testlist.
Regards,
Shafi
