Hi Serguei,
Please review the new version of the fix that corrects the order of include
statements in src/hotspot/share/runtime/notificationThread.cpp.
The list of Include statements doesn't contain "#include
"runtime/mutexLocker.hpp" since this include file is already included by
runtime/interfaceSupport.inline.hpp that is in this list.
I don't think we need the following function:
static bool is_notification_thread(Thread* thread);
For the ServiceThread the function is_service_thread(Thread* thread) is used
only once in the code. It is used inside JVmtiDeferredEvent::post() to assert
that the proper thread is used to post these events. Low memory, GC and
diagnostic command notification never had such asserts so I'm not sure we need
to introduce them regarding new NotificationThread.
Webrev: https://cr.openjdk.java.net/~dtitov/8170299/webrev.02/
Bug: https://bugs.openjdk.java.net/browse/JDK-8170299
Thanks!
--Daniil
On 7/3/19, 9:02 PM, "serguei.spit...@oracle.com" <serguei.spit...@oracle.com>
wrote:
Hi Daniil,
I've not finished my review but it looks good in general.
A couple of quick comments.
https://cr.openjdk.java.net/~dtitov/8170299/webrev.01/src/hotspot/share/runtime/notificationThread.hpp.html
I wonder if this function is also needed:
static bool is_notification_thread(Thread* thread);
https://cr.openjdk.java.net/~dtitov/8170299/webrev.01/src/hotspot/share/runtime/notificationThread.cpp.html
I wonder why this include statement is missed:
#include "runtime/mutexLocker.hpp"
Also, these have to be correctly ordred:
29 #include "runtime/notificationThread.hpp"
30 #include "services/lowMemoryDetector.hpp"
31 #include "services/gcNotifier.hpp"
32 #include "services/diagnosticArgument.hpp"
33 #include "services/diagnosticFramework.hpp"
Thanks,
Serguei
On 7/3/19 8: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.
>
> 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
>
>
