Hi Chris,
On 3/08/2019 4:12 am, Chris Plummer wrote:
On 7/31/19 11:50 PM, David Holmes wrote:
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.
This is indeed problematic, but it seems less of an issue than running
java code on the service thread. BTW, are there any other cases where we
run java code on the service thread? It seems running java code on a
hidden thread is just asking for trouble. I assume if you hit a
breakpoint while doing this, it is simply ignored. Not exactly what the
debugger user is expecting.
The ServiceThread was introduced as a generalization of the
LowMemoryDetectorThread, specifically because it was needed to run Java
code (in particular load Java classes) in response to other event
notifications (ie compiler load events). See JDK-6766644 fixed in JDK 7.
So it has always been the case that the low memory notification code has
executed in a hidden thread; and since JDK-6766644 other Java code has
executed in this same hidden thread.
As I said at the start this bug has a long and complex history. It is
far from clear that it can, or even should, be fixed, due to the other
compatibility problems that the fix will introduce.
If you look at the docs for MemoryPoolMXBean:
https://docs.oracle.com/javase/10/docs/api/java/lang/management/MemoryPoolMXBean.html
it recommends doing minimal work in the actual notification code and
instead hand off the real work to another thread:
"The handleNotification method should be designed to do a very minimal
amount of work and return without delay to avoid causing delay in
delivering subsequent notifications. Time-consuming actions should be
performed by a separate thread."
with that approach you don't need to be able to debug the notification
code executed by the service-thread because it should be trivial.
I would agree that the threading model for the notification system is
under-specified and that these kinds of details should have been
considered originally and any limitations clearly spelt out. Perhaps all
we should do here is improve the documentation?
Either way any change in doc or behaviour will require a CSR request.
Cheers,
David
-----
Chris
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.hol...@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
>
>