PING: RFR: 8219566: JFR did not collect call stacks when MaxJavaStackTraceDepth is set to zero

Sun Apr 14 22:42:59 UTC 2019

Looks good (webrev.03)

According to the option spec. MaxJavaStackTraceDepth controls "the maximum number of lines in the stack trace for Java exceptions”. This is not an exception stack trace, so I think it would be more appropriate to use MAX_STACK_DEPTH.

Thanks
Erik

> On 14 Apr 2019, at 15:05, Yasumasa Suenaga <yasuenag at gmail.com> wrote:
> 
> Hi Erik,
> 
> How about this change?
>  http://cr.openjdk.java.net/~ysuenaga/JDK-8219566/webrev.03/
> 
> IMHO I prefer webrev.00 if we need to think stackdepth is set to lower value.
> Fix in webrev.00 is introduced in JDK-7179701.
> 
> As you said before, it does not make the code easier to read.
> 
> So if webrev.03 is appropriate, I will push webrev.03.
> 
> 
> Thanks,
> 
> Yasumasa
> 
> 
> On 2019/04/13 3:00, Erik Gahlin wrote:
>> Hi,
>> Could we use the max stack depth used by JFR instead?
>> It's not available as constant in a header file, but maybe you could move:
>> static const u4 STACK_DEPTH_DEFAULT = 64;
>> static const u4 MIN_STACK_DEPTH = 1;
>> static const u4 MAX_STACK_DEPTH = 2048;
>> from jfrOptionSet.cpp to jfrTypes.hpp where we have other constants. The benefit of using MAX_STACK_DEPTH is that it will work in scenarios where a user has set the number of frames to a low  number, i.e -XX:FlightRecorderOptions=5, and we hit a stack with lots of frames that we can't record, i.e native frames.
>> Thanks
>> Erik
>>> Hi,
>>> 
>>> Do you have any concern(s) about this change?
>>> 
>>>>>>    webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8219566/webrev.02/
>>> 
>>> I think thread stack should be collected by JFR even if MaxJavaStackTraceDepth is set to zero.
>>> 
>>> 
>>> Thanks,
>>> 
>>> Yasumasa
>>> 
>>> 
>>> On 2019/03/23 21:03, Yasumasa Suenaga wrote:
>>>> Hi Erik,
>>>> 
>>>> MaxJavaStackTraceDepth do not only affect finding top stack but also stack walking.
>>>> 
>>>> JfrGetCallTrace::find_top_frame() uses MaxJavaStackTraceDepth to find top frame as you say.
>>>> OTOH vframeStreamSamples::samples_next() uses it to find next valid frame.
>>>> 
>>>> 
>>>> Anyway thread stack should be collected even if MaxJavaStackTraceDepth is set to zero.
>>>> 
>>>> 
>>>> Thanks,
>>>> 
>>>> Yasumasa
>>>> 
>>>> 
>>>> On 2019/03/23 4:25, Erik Gahlin wrote:
>>>>> To me, it seems that the purpose of the method is to find the first
>>>>> valid frame on the stack (the top frame). This is orthogonal to the
>>>>> number of frames that should be recorded by JFR.
>>>>> 
>>>>> Erik
>>>>> 
>>>>>> PING: Could you review it?
>>>>>> 
>>>>>>    JBS: https://bugs.openjdk.java.net/browse/JDK-8219566
>>>>>>    webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8219566/webrev.02/
>>>>>> 
>>>>>> This change has passed :jdk_jfr tests and submit repo.
>>>>>> 
>>>>>> 
>>>>>> Thanks,
>>>>>> 
>>>>>> Yasumasa
>>>>>> 
>>>>>> 
>>>>>> On 2019/03/11 14:49, Yasumasa Suenaga wrote:
>>>>>>> 2019?3?9?(?) 20:47 Yasumasa Suenaga <yasuenag at gmail.com>:
>>>>>>>> 
>>>>>>>> Hi Erik,
>>>>>>>> 
>>>>>>>> Reading your email, I think we should use JfrOptionSet::stackdepth()
>>>>>>>> rather than MaxJavaStackTraceDepth.
>>>>>>>> I created new webrev, and it passed jdk_jfr jtreg tests on my Linux
>>>>>>>> x64 box.
>>>>>>>> 
>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8219566/webrev.01/
>>>>>>>> 
>>>>>>>> However it has not yet passed tests on submit repo
>>>>>>>> (mach5-one-ysuenaga-JDK-8219566-20190309-0909-1079793).
>>>>>>>> It seems to be failed at build phase which includes Linux.
>>>>>>>> (I can build with this change on Fedora 29 x64).
>>>>>>>> 
>>>>>>>> Could you help if this webrev is ok?
>>>>>>> 
>>>>>>> I found 1 warning, and I fixed it.
>>>>>>> This webrev works fine with :jdk_jfr jtreg tests on my Linux box, and
>>>>>>> submit repo test
>>>>>>> (mach5-one-ysuenaga-JDK-8219566-20190311-0150-1121570).
>>>>>>> Could you review it?
>>>>>>> 
>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8219566/webrev.02/
>>>>>>> 
>>>>>>> 
>>>>>>> Thanks,
>>>>>>> 
>>>>>>> Yasumasa
>>>>>>> 
>>>>>>> 
>>>>>>>> Thanks,
>>>>>>>> 
>>>>>>>> Yasumasa
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On 2019/03/09 0:37, Erik Gahlin wrote:
>>>>>>>>> JFR imposes a limit on how many frames that can be recorded, set to
>>>>>>>>> 64 by default but configurable up to 2048 using
>>>>>>>>> -XX:FlightRecorderOptions, so setting the value to 0 will still not
>>>>>>>>> mean all frames.
>>>>>>>>> 
>>>>>>>>> That said, if you still want to fix it. It may make sense to set
>>>>>>>>> loop_max to INT_MAX, if MaxJavaStackTraceDepth is zero, i.e
>>>>>>>>> 
>>>>>>>>> int loop_max = MaxJavaStackTraceDepth == 0 ? INT_MAX :
>>>>>>>>> MaxJavaStackTraceDepth * 2;
>>>>>>>>> 
>>>>>>>>> and avoid evaluating it for every frame. Even if the compiler is
>>>>>>>>> able to hoist the comparison, it makes the code easier to read.
>>>>>>>>> 
>>>>>>>>> Thanks
>>>>>>>>> Erik
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>>> PING: Could you review it?
>>>>>>>>>> 
>>>>>>>>>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8219566
>>>>>>>>>>>>>     webrev:
>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8219566/webrev.00/
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> If you have any questions, please tell me.
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> Thanks,
>>>>>>>>>> 
>>>>>>>>>> Yasumasa
>>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> On 2019/02/27 22:46, Yasumasa Suenaga wrote:
>>>>>>>>>>> Hi Erik,
>>>>>>>>>>> 
>>>>>>>>>>> On 2019/02/27 22:30, Erik Gahlin wrote:
>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>> 
>>>>>>>>>>>> Could you explain a little about the bug?
>>>>>>>>>>>> 
>>>>>>>>>>>> Why should stack traces be collected if MaxJavaStackTraceDepth
>>>>>>>>>>>> is set to 0.
>>>>>>>>>>> 
>>>>>>>>>>> "0" in MaxJavaStackTraceDepth means all exception stack traces
>>>>>>>>>>> will be shown.
>>>>>>>>>>> 
>>>>>>>>>>> http://hg.openjdk.java.net/jdk/jdk/file/72ce7dd54939/src/hotspot/share/runtime/globals.hpp#l1553
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> JDK-7179701 reports similar issue.
>>>>>>>>>>> I think we can take similar approach for JDK-8219566.
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> Thanks,
>>>>>>>>>>> 
>>>>>>>>>>> Yasumasa
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>>> Thanks
>>>>>>>>>>>> Erik
>>>>>>>>>>>> 
>>>>>>>>>>>>> On 22 Feb 2019, at 07:48, Yasumasa Suenaga <yasuenag at gmail.com>
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Please review this change:
>>>>>>>>>>>>> 
>>>>>>>>>>>>>     JBS: https://bugs.openjdk.java.net/browse/JDK-8219566
>>>>>>>>>>>>>     webrev:
>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8219566/webrev.00/
>>>>>>>>>>>>> 
>>>>>>>>>>>>> JFR did not collect call stacks when MaxJavaStackTraceDepth is
>>>>>>>>>>>>> set to zero.
>>>>>>>>>>>>> This bug is similar to JDK-7179701.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> This change passed tests on submit repo and :jdk_jfr jtreg tests.
>>>>>>>>>>>>> 
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> 
>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>> 
>>>>>>>>> 
>>>>> 