RFR: 8170299: Debugger does not stop inside the low memory notifications code

[David Holmes]

Hi Daniil,

On 25/07/2019 3:34 am, Daniil Titov wrote:
> Hi David,
> 
> Hope you had a great vacation!

I did thank you. Apologies again for taking so long to get back to this 
work.

> Please find below the latest version of the change . The only difference from the version 01 is
> the corrected ordering of include statements as Serguei suggested.
> 
> Webrev: https://cr.openjdk.java.net/~dtitov/8170299/webrev.02/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8170299

I'm still remain concerned about introducing yet-another-thread to the 
system. The potential interactions with other threads is not at all clear.

I'm also concerned that this thread has to be visible so that you can 
debug the notification code, yet at the same time being visible makes it 
vulnerable to application level actions that don't impact the service 
thread - in particular if we suspend all threads then this thread will 
be suspended too, if we resume a thread that triggers a notification, 
the notification thread won't be able to respond to it as it is 
suspended. The user won't know that they need to explicitly resume this 
internal system thread.

Also note in serviceThread.cpp we have:

  129       // This ThreadBlockInVM object is not also considered to be
  130       // suspend-equivalent because ServiceThread is not visible to
  131       // external suspension.
  132
  133       ThreadBlockInVM tbivm(jt);

and you copied that across to notificationThread.cpp as:

   93       // Need state transition ThreadBlockInVM so that this thread
   94       // will be handled by safepoint correctly when this thread is
   95       // notified at a safepoint.
   96
   97       ThreadBlockInVM tbivm(jt);

so this will continue to not be a suspend-equivalent condition even 
though this thread is visible and suspendible! So something seems wrong 
there. I'm unclear why we need to use the ThreadBlockInVM rather than 
defining the NotificationLock as a safepoint-checks-always lock, rather 
than a safepoint-check-never lock? In fact with some recent changes to 
locks I'm not even sure it is legal for the notification thread to use a 
safepoint-check-never lock - have you re-based this recently?

Thanks,
David

> Thanks!
> --Daniil
> 
> On 7/3/19, 11:47 PM, "David Holmes" <david.holmes at oracle.com> wrote:
> 
>      Hi Daniil,
>      
>      On 4/07/2019 1:04 pm, Daniil Titov wrote:
>      > Please review the change the fixes the problem with the debugger not stopping in the low memory notification code.
>      >
>      > The problem here is that the ServiceThread that calls these MXBean listeners is hidden from the external view that prevents the debugger from stopping in it.
>      >
>      > The fix introduces new NotificationThread that is visible to the external view and offloads the ServiceThread from sending low memory and other notifications that could result in Java calls ( GC and diagnostic commands notifications) by moving these activities in this new NotificationThread.
>      
>      There is a long and unfortunate history with this bug.
>      
>      The original incarnation of this fix was introducing a new thread at the
>      Java library level, and I had some concerns about that:
>      
>      http://mail.openjdk.java.net/pipermail/serviceability-dev/2017-December/022612.html
>      
>      That effort was resurrected at:
>      
>      http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-July/024466.html
>      
>      and
>      
>      http://mail.openjdk.java.net/pipermail/serviceability-dev/2018-August/024849.html
>      
>      but was left somewhat in limbo. There was a lot of doubt about the right
>      way to fix this bug and whether introducing a new thread was too disruptive.
>      
>      But introducing a new thread in the VM also has the same set of
>      concerns! This needs consideration by the runtime team before going
>      ahead. Introducing a new thread likes this needs to be examined in
>      detail - particularly the synchronization interactions with other
>      threads. It also introduces another monitor designated safepoint-never
>      at a time when we are in the process of cleaning up monitors so that
>      JavaThreads will only use safepoint-check-always monitors.
>      
>      Unfortunately I'm about to head out for two weeks vacation, and a number
>      of other key runtime folk are also on vacation. but I'd ask that you
>      hold off on this until we can look at it in more detail.
>      
>      Thanks,
>      David
>      -----
>      
>      > Testing: Mach5 tier1,tier2 and tier3 tests succeeded.
>      >
>      > Webrev: https://cr.openjdk.java.net/~dtitov/8170299/webrev.01/
>      > Bug: https://bugs.openjdk.java.net/browse/JDK-8170299
>      >
>      > Thanks!
>      > --Daniil
>      >
>      >
>      
> 
>