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