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

[Denghui Dong]

Thanks! Please help me to push it if everything is ok.

Denghui Dong





来自钉钉专属商务邮箱------------------------------------------------------------------
发件人：Erik Gahlin<erik.gahlin at oracle.com>
日　期：2020年01月29日 05:50:57
收件人：Denghui Dong<denghui.ddh at alibaba-inc.com>; hotspot-jfr-dev<hotspot-jfr-dev at openjdk.java.net>
主　题：Re: RFR: 8237499: JFR event for the location where a new thread starts

    
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
 >