RFR: 8237499: JFR event for the location where a new thread starts

[Erik Gahlin]

Hi Denghui,

On 2020-01-24 04:44, Denghui Dong wrote:
> Hi Erik,
>  > set stackTrace="true" in metadata.xml
>  Actually, I thought about it before. But two thing let me add a new 
> field explicitly finally:
> 1. EventThreadStart was also posted in jni.cpp, and there is no need 
> to record the stack trace, we cannot control it if setting 
> stackTrace="true".
>
Not sure I understand, but in jni.cpp you could do what you do today 
(set it to 0).

For jfrThreadLocal.cpp, you could check 
JfrEventSetting::has_stacktrace(EventThreadStart::eventId)).

>
> 2.  I think adding an explicit filed can make us and users clear that 
> the stack trace isn't recorded from the new thread but the parent 
> thread. And
> there are also other events that have an explicit stack trace 
> field(e.g. ExecutionSample).
>
>
ExecutionSample and NativeMethodSample should probably have 
stackTrace="true" as well.

I don't think it matters from users point of view if the field is added 
implicitly or explicitly. Maybe if it was called "parentStackTrace", but 
that would lead to problems for JMC and the parser since they depend on 
the field being called "stackTrace". For us, it would make it more 
clear, but perhaps not that important.


Another reason we may want to make the stack trace part of the event is 
that it will make it easier to configure. For example, we plan to add so 
you can set stack depth per event, for example in the JMC recording wizard.
https://bugs.openjdk.java.net/browse/JDK-8134306

Thanks
Erik

>
>  > change the label of the thread field from "Java Thread" to "New Thread"
>  It makes sense, I will update it.
>
> Thanks
> Denghui Dong
>
>     ------------------------------------------------------------------
>     From:Erik Gahlin <erik.gahlin at oracle.com>
>     Send Time:2020年1月23日(星期四) 21:44
>     To:董登辉(卓昂) <denghui.ddh at alibaba-inc.com>; hotspot-jfr-dev
>     <hotspot-jfr-dev at openjdk.java.net>
>     Subject:Re: RFR: 8237499: JFR event for the location where a new
>     thread starts
>
>     Hi Denghui,
>
>     Would it be possible to set stackTrace="true" in metadata.xml
>     instead of adding a new field explicitly?
>
>     This allows users to turn on and off stack traces. You could check
>     if stack trace is enabled and only record if needed.
>
>     I think it would be good to change the label of the thread field
>     from "Java Thread" to "New Thread".
>
>     Thanks
>     Erik
>
>     On 2020-01-19 09:13, Denghui Dong wrote:
>     Hi Erik
>       The webrev has been updated.
>       I reuse the event type "EventThreadStart" to report the location
>     where a new thread starts.
>       Add two fields "stackTrace" and "parentThread" to EventThreadStart.
>       Please help review it, thanks.
>
>     Best Regards
>     Denghui Dong
>     ------------------------------------------------------------------
>     From:董登辉(卓昂) <denghui.ddh at alibaba-inc.com>
>     Send Time:2020年1月18日(星期六) 21:25
>     To:Erik Gahlin <erik.gahlin at oracle.com>; hotspot-jfr-dev
>     <hotspot-jfr-dev at openjdk.java.net>
>     Subject:Re: RFR: 8237499: JFR event for the location where a new
>     thread starts
>
>     Hi Erik,
>       Do you mean that we should reuse EventThreadStart rather than a
>     new event? It's a good idea, I will try it.
>
>     Thanks
>     Denghui Dong
>     ------------------------------------------------------------------
>     From:Erik Gahlin <erik.gahlin at oracle.com>
>     Send Time:2020年1月18日(星期六) 21:12
>     To:董登辉(卓昂) <denghui.ddh at alibaba-inc.com>
>     Cc:hotspot-jfr-dev <hotspot-jfr-dev at openjdk.java.net>
>     Subject:Re: RFR: 8237499: JFR event for the location where a new
>     thread starts
>
>     Hi Denghui,
>
>     This will be useful, but I wonder if we shouldn’t extend the Thread Start event with stack trace and the parent thread? It will require some additional logic, but it would be a cleaner solution.
>
>     Thanks
>     Erik
>
>     > On 18 Jan 2020, at 12:19, Denghui Dong
>     <denghui.ddh at alibaba-inc.com> wrote:
>     >
>     > Hi,
>     >
>     > Could I have a review of a change that introduces an event for the location where a new thread starts.
>     >
>     > Summary:
>     > There is already an event for thread start(EventThreadStart), but this event is committed in the new thread,
>     > so users can't know the location where the thread starts, hence, it's necessary to introduce another event,
>
>     > it would be useful if the application exists thread leak problem.
>     >
>     > Bug: https://bugs.openjdk.java.net/browse/JDK-8237499
>     > Webrev: http://cr.openjdk.java.net/~ddong/8237499/
>     >
>     > Thanks
>     > Denghui Dong
>     > 
>
>