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

[Yasumasa Suenaga]

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