PING: RFR: 8219566: JFR did not collect call stacks when MaxJavaStackTraceDepth is set to zero
Markus Gronlund
markus.gronlund at oracle.com
Tue Apr 23 12:37:38 UTC 2019
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