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

[Erik Gahlin]

Looks good!

Thanks for fixing this.

Erik

On 2020-01-28 18:07, Denghui Dong wrote:
> Good explanation!
> The webrev has been updated, please help review it :)
>
> Denghui Dong
>
>     ------------------------------------------------------------------
>     From:Erik Gahlin <erik.gahlin at oracle.com>
>     Send Time:2020年1月28日(星期二) 20:38
>     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,
>
>     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
>     >
>