PING: RFR: 8219566: JFR did not collect call stacks when MaxJavaStackTraceDepth is set to zero
Yasumasa Suenaga
yasuenag at gmail.com
Wed Apr 24 01:51:57 UTC 2019
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