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

Sun Apr 14 13:05:22 UTC 2019

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