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

[Erik Gahlin]

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
>>>>>>>>>>
>>>>>>>
>>>