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

[Thomas Stüfe]

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> 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>
> Sent: Freitag, 1. März 2019 01:53
> To: Thomas Stüfe <thomas.stuefe at gmail.com>; Doerr, Martin <
> martin.doerr at sap.com>
> Cc: 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
> -----
>