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

Markus Gronlund MARKUS.GRONLUND at ORACLE.COM
Wed Apr 24 07:44:56 UTC 2019


Ok, looks good.

Markus

Sent from my iPhone

> On 24 Apr 2019, at 03:51, Yasumasa Suenaga <yasuenag at gmail.com> wrote:
> 
> Hi Markus,
> 
> Thank you for your comment.
> 
> I fixed them in webrev.04 . Could you review again?
> 
>  http://cr.openjdk.java.net/~ysuenaga/JDK-8219566/webrev.04/
> 
> 
> Yasumasa
> 
> 
>> On 2019/04/23 21:37, Markus Gronlund wrote:
>> src/hotspot/share/jfr/periodic/sampling/jfrCallTrace.cpp
>>      Need #include "jfr/utilities/jfrTypes.hpp"
>> src/hotspot/share/jfr/recorder/service/jfrOptionSet.cpp
>>      Need #include "jfr/utilities/jfrTypes.hpp"
>> src/hotspot/share/jfr/recorder/stacktrace/jfrStackTraceRepository.cpp
>>      loop_count and loop_max might need to have type u4 instead of int to avoid unsigned / signed type warning / errors
>> src/hotspot/share/jfr/utilities/jfrTypes.hpp
>>      No comment.
>> Thanks
>> Markus
>> -----Original Message-----
>> From: Yasumasa Suenaga <yasuenag at gmail.com>
>> Sent: den 22 april 2019 12:23
>> To: hotspot-jfr-dev at openjdk.java.net
>> Subject: PING: RFR: 8219566: JFR did not collect call stacks when MaxJavaStackTraceDepth is set to zero
>> PING: Could you review it?
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8219566
>> webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8219566/webrev.03/
>> I need one more reviewer to push.
>> Yasumasa
>>> On 2019/04/15 8:14, Yasumasa Suenaga wrote:
>>> Thanks Erik!
>>> 
>>> I'm waiting for another reviewer for webrev.03 .
>>> 
>>> 
>>> Yasumasa
>>> 
>>> 
>>> 2019年4月15日(月) 7:43 Erik Gahlin <erik.gahlin at oracle.com
>>> <mailto:erik.gahlin at oracle.com>>:
>>> 
>>>     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
>>>     <mailto: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
>>>     <http://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 <http://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
>>>      >>>>>>>>>>>>
>>>      >>>>>>>>>
>>>      >>>>>
>>> 



More information about the hotspot-jfr-dev mailing list