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

[David Holmes]

Hi Martin,

Sorry for the delayed response.

On 4/03/2019 11:21 pm, Doerr, Martin 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()).

Not quite what I'd hoped as I thought we already used a platform 
specific function to print the signal information - but we don't, so okay.

>> 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.

That doesn't quite read correctly, I'd just drop the "because" and use a 
colon. No need for updated webrev for that.

> 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.

Saw that review thread - thanks.

No further comments from me.

Thanks,
David
-----

> 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
> -----
>