Re: [8u] [JFR] RFR: 8236160: Missing ThreadCrashProtection for JFR in signal handler

[Denghui Dong]

Hi Andrew,
  Thanks for the reply.
  As I said in my last message, merging os::ThreadCrashProtection and os::WatcherThreadCrashProtection is a more elegant way,
actually this work was done by JDK-8183925(Decouple crash protection from watcher thread), but it's not JFR-related.
  And the first patch of JFR-backport use os::ThreadCrashProtection to protect jfr thread sampler crash, but miss check_crash_protection in the signal handler,
so I made the changeset.
  Do you think we should backport JDK-8183925 to jdk8u-jfr-incubator?

Best Regards,
Denghui Dong
------------------------------------------------------------------
From:Andrew Dinn <adinn at redhat.com>
Send Time:2019年12月19日(星期四) 01:17
To:董登辉(卓昂) <denghui.ddh at alibaba-inc.com>; jdk8u-dev <jdk8u-dev at openjdk.java.net>
Subject:Re: [8u] [JFR] RFR: 8236160: Missing ThreadCrashProtection for JFR in signal handler

Hi Denghui,

On 18/12/2019 06:37, Denghui Dong wrote:
> Hi all,
>   Please help review the following changeset:
> Jira: https://bugs.openjdk.java.net/browse/JDK-8236160
> Webrev: http://cr.openjdk.java.net/~ddong/8236160/webrev.00/hotspot.changeset
> Summary:
> JFR thread sampler uses ThreadCrashProtection to protect thread crash, so it's necessary to call
> ThreadCrashProtection::check_crash_protection in signal handler.
> And a more elegant way is to merge ThreadCrashProtection and WatcherThreadCrashProtection, but this changeset didn't do that.
I'm not clear why you are calling both

  os::WatcherThreadCrashProtection::check_crash_protection(sig, t);

and

  os::ThreadCrashProtection::check_crash_protection(sig, t);

Is it not possible simply to rely on os::ThreadCrashProtection alone as
happens updtream?

I am assuming that you have already back-ported class
os::ThreadCrashProtection to the JFR incubator repo? Did you need to
change it's behaviour in any way in order to do that? Does that have
anything to do with why your are calling both routines?

Also, how have you tested this patch?

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill