RFR (XS) 8213992: Rename and make DieOnSafepointTimeout the diagnostic option

[David Holmes]

Hi Aleksey,

On 17/11/2018 2:18 am, Aleksey Shipilev wrote:
> RFE:
>    https://bugs.openjdk.java.net/browse/JDK-8213992
> 
> While the original option was apparently conceived to aid debugging during the VM development, it is
> also useful in product builds to terminate VM when it is stuck on safepoint sync. This is sometimes
> asked by external people who have fail-overs set up to deal with VM crashes: making VM fail
> predictably on errors like this is a plus for them.
> 
> David suggested making the option "product", but I don't like it very much: "product" option
> suggests this is a production-grade feature and it comes with expectations of support, which is
> interesting in itself when product feature crashes the VM. "diagnostic" keeps this mode available in
> product builds without the attached notion of support. Users that want fail-fast VM crash can then
> use that option on "we know what we are doing" basis.

I feel a little uncomfortable that "diagnostic" is being used as a 
synonym for "unsupported product". If you really want to fail-fast in 
production then you're looking for a production flag not a "diagnostic" 
one IMHO. I won't fight it but I'd like to know what others think.

> The name of the option is hopefully aligned with:
>    diagnostic AbortVMOnException (existing)
>    diagnostic AbortVMOnSafepointTimeout (this one)
>    diagnostic AbortVMOnVMOperationTimeout (JDK-8181143)
>    diagnostic AbortVMOnCompilationFailure (JDK-8212070)

That's just crying out for an AbortVMOn:<event> type of interface. :) 
But I won't force that on you.

> Fix:
> 
> diff -r 9ad663e63da5 -r 132db6e99f77 src/hotspot/share/runtime/globals.hpp
> --- a/src/hotspot/share/runtime/globals.hpp	Fri Nov 16 12:02:08 2018 +0100
> +++ b/src/hotspot/share/runtime/globals.hpp	Fri Nov 16 13:35:16 2018 +0100
> @@ -498,7 +498,7 @@
>             "Time out and warn or fail after SafepointTimeoutDelay "          \
>             "milliseconds if failed to reach safepoint")                      \
>                                                                               \
> -  develop(bool, DieOnSafepointTimeout, false,                               \
> +  diagnostic(bool, AbortVMOnSafepointTimeout, false,                        \
>             "Die upon failure to reach safepoint (see SafepointTimeout)")     \
s/Die/Abort

Thanks.
 
        \
>     /* 50 retries * (5 * current_retry_count) millis = ~6.375 seconds */      \
> diff -r 9ad663e63da5 -r 132db6e99f77 src/hotspot/share/runtime/safepoint.cpp
> --- a/src/hotspot/share/runtime/safepoint.cpp	Fri Nov 16 12:02:08 2018 +0100
> +++ b/src/hotspot/share/runtime/safepoint.cpp	Fri Nov 16 13:35:16 2018 +0100
> @@ -978,9 +978,9 @@
>       }
>     }
> 
> -  // To debug the long safepoint, specify both DieOnSafepointTimeout &
> +  // To debug the long safepoint, specify both AbortVMOnSafepointTimeout &
>     // ShowMessageBoxOnError.
> -  if (DieOnSafepointTimeout) {
> +  if (AbortVMOnSafepointTimeout) {
>       fatal("Safepoint sync time longer than " INTX_FORMAT "ms detected when executing %s.",
>             SafepointTimeoutDelay, VMThread::vm_safepoint_description());
>     }
> 
> Testing: hotspot/tier1, grepping for DieOnSafepointTimeout usages, jdk-submit (running)

No further comments.

Thanks,
David

> Thanks,
> -Aleksey
>