RFR(M): 8219584: Try to dump error file by thread which causes safepoint timeout

[Doerr, Martin]

Hi Thomas,

renamed in place as requested by you (webrev.02). Makes sense. Thanks for the review.
I’ll request a test review on hotspot-compiler-dev.

Best regards,
Martin


From: Thomas Stüfe <thomas.stuefe at gmail.com>
Sent: Mittwoch, 6. März 2019 06:29
To: Doerr, Martin <martin.doerr at sap.com>
Cc: David Holmes <david.holmes at oracle.com>; hotspot-runtime-dev at openjdk.java.net
Subject: Re: RFR(M): 8219584: Try to dump error file by thread which causes safepoint timeout

Hi Martin,

modulo Davids concerns regarding the test case,  this looks good so far, with one nit:

Please rename "signal_sent_by_other_thread" to "signal_sent_by_kill" and change the printout in vmError.cpp accordingly.

Signal could be sent by other thread or process.

Cheers, Thomas


On Mon, Mar 4, 2019 at 2:21 PM Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>> wrote:
Hi David,

> I like the general functionality but not the code placement. :)
Thanks. Right, the placement should be improved. Thanks for your advice.
Moved "os::signal_thread" to os_posix/os_windows as you suggested.

> I don't see why any ifdef is needed in safepoint.cpp ...
Removed ifdef _WINDOWS from safepoint.cpp. SIGILL is defined on Windows, so this should be ok.

> ... I really do not like the ifdefs added to
> vmError.cpp. This should be printed as part of the signal info and
> handled in the appropriate platform code.
I don't like them, either ��
My new proposal only adds "(sent by other thread)" to the "#" prefixed header.
Details can be found under "printing siginfo" step (factored out detection os::signal_sent_by_other_thread()).

> In signal_thread why are we using Event::log? I'm not familiar with it.
It results in an entry in the section "Events (10 events):":
Event: ... Thread ... sent signal 4 to Thread ... because blocking a safepoint.
It can provide an additional hint to understand why the signal occurred.

> doesn't the safepoint polling depend on the value of
> UseCountedLoopSafepoints? I'd want a compiler person to check that this
> test is reliably going to check what you want it to check.
Actually, LoopStripMiningIter is the flag which eventually influences C2, but you're right,
UseCountedLoopSafepoints should be forced off to prevent some internal logic from changing LoopStripMiningIter.
I can ask somebody from the Compiler Team to provide a statement when the Runtime Team is happy with the rest.

New webrev:
http://cr.openjdk.java.net/~mdoerr/8219584_kill_thread_on_safepoint_timeout/webrev.02/

Thanks for reviewing.
Best regards,
Martin


-----Original Message-----
From: David Holmes <david.holmes at oracle.com<mailto:david.holmes at oracle.com>>
Sent: Freitag, 1. März 2019 01:53
To: Thomas Stüfe <thomas.stuefe at gmail.com<mailto:thomas.stuefe at gmail.com>>; Doerr, Martin <martin.doerr at sap.com<mailto:martin.doerr at sap.com>>
Cc: hotspot-runtime-dev at openjdk.java.net<mailto:hotspot-runtime-dev at openjdk.java.net>
Subject: Re: RFR(M): 8219584: Try to dump error file by thread which causes safepoint timeout

Hi Martin,

I like the general functionality but not the code placement. :) It';s
easiest to build on Thomas's response below ...

On 28/02/2019 12:57 am, Thomas Stüfe wrote:
> Hi Martin,
>
> I like this. I have minor points, most of them a matter of taste:
>
> ¹) I actually do not like much having os::signal_thread(int sig).
> Signals are a Posix concept, there is nothing on windows like it. That

Perhaps not thread-directed signals but still:

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/signal?view=vs-2017

I don't have any concern about this being an os function even if the
Windows version is basically a no-op ("return false;"). However it
should not be defined in os.cpp with a bunch of ifdefs! That's why we
have the os_<os>.cpp files. There should be one version in os_posix.cpp
(may as well keep the Solaris ifdefs there) and one in os_windows.cpp.

> is why we still need +#ifndef _WINDOWS in safepoint.cpp, even though
> os::signal_thread() is available on windows too.

I don't see why any ifdef is needed in safepoint.cpp, on windows we'll
break out of the loop on the first iteration. We shouldn't care about
overhead here (and maybe someday windows signal_thread will do something).

<snip>

>
> 2) in vmError.cpp, you want to have the signal info more prominently,
> which I like. But I would just call print_siginfo() there. It is defined
> to print a single line without /cr, so it should fit nicely into the
> "#.." section.
> If you do not like the idea, you would have to not only handle TKILL but
> also SI_KILL and SI_QUEUE (I think), duplicating the work in
> print_siginfo. Note that TKILL is, I think, only available on Linux.

I think I agree with Thomas. I really do not like the ifdefs added to
vmError.cpp. This should be printed as part of the signal info and
handled in the appropriate platform code.

Other comments:

In signal_thread why are we using Event::log? I'm not familiar with it.

In the test:

   52             // Long running loop without safepoint.
   53             for (int y = 1; y < Integer.MAX_VALUE; ++y) {
   54                 if (y % x == 0) ++sum;
   55             }

doesn't the safepoint polling depend on the value of
UseCountedLoopSafepoints? I'd want a compiler person to check that this
test is reliably going to check what you want it to check.

Thanks,
David
-----