On Wed, 21 Oct 2026 01:39:19 GMT, Xin Liu <[email protected]> wrote: >> Roman Kennke has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Improve TestRequestStackTraceForeignThread to be more reliable under load > > src/hotspot/share/jfr/periodic/sampling/jfrStackWalker.cpp line 608: > >> 606: } >> 607: >> 608: static bool build_from_ljf(JfrStackWalkRequest& request, > > This function is almost identical to jfrSampleRequest.cpp/build_from_ljf. > Should we combine them? it's hard to sell that we have two parallel lines in > JFR.
Good point! Merging them also turned up a real bug: the walker's copy of compute_top_frame predated the JEP 401 integration and lacked the NEEDS_STACK_REPAIR handling. Since CPU-time sampling now goes through the walker, that crashed reliably with value classes (new test TestCPUTimeSampleNeedStackRepair, a CPU-time variant of TestNeedStackRepair). The walker now reuses JfrSampleRequestBuilder (replacing the unused build_cpu_time_sample_request with build_unbiased_sample_request), JfrThreadSampling::compute_top_frame and JfrStackTrace::record. Its own vframe stream, reporting loop and vthread tracking are gone too. That's about 600 lines less, and fewer changes to existing JFR code than before. > src/hotspot/share/jfr/periodic/sampling/jfrStackWalker.cpp line 939: > >> 937: >> 938: in_continuation = is_in_continuation(top_frame, jt); >> 939: return true; > > There's a potential risk for 'top_frame'. I know it's not this PR introduces > it. just call it out. > > We actually generate an empty top_frame at line 1033. Currently, it's > defined. Everything is null because all platforms define its constructor > frame::frame(). > > I think it's possible compute_top_frame() returns true but never get assigned > by any valid frame. eg. stream.is_done() == true so we skip the loop > entirely. > > 1. skip to report failure because it returns true. > 2. JfrStackWalkerVframeStream will walk the stack starting with this dummy > frame. I am not sure it's 100% safe. I fixed this, thanks for the heads-up! > src/hotspot/share/runtime/continuationFreezeThaw.cpp line 604: > >> 602: assert_frames_in_continuation_are_safe(_thread); >> 603: JFR_ONLY(JfrStackWalker::check_and_process_requests(_thread)); >> 604: JFR_ONLY(Jfr::check_and_process_sample_request(_thread)); > > Can we consider merge JfrStackWalker::check_and_process_requests(jt) into > Jfr::check_and_process_sample_request(jt)? > > In the prior PR, JVMTI RequestStackTrace was an independent feature. now it > depends on JFR in runtime. We can't use it without --enable-jfr. I would say > that we just embrace that. > > 1. If we only update `Jfr::check_and_process_sample_request(jt)`, we will > shrink the patch size. We don't need to update runtime functions. The effect > is same. You can check JFRStackWalker first. We can also avoid including > details like jfrStackWalker.ineline.hpp. > > 2. One utility function is also good for maintenance. We will never forget > they appear in pairs. Right, good call! I reverted this, and also minimized the JFR exposure of this PR in a few other places. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/31059#discussion_r4352335918 PR Review Comment: https://git.openjdk.org/jdk/pull/31059#discussion_r4352335503 PR Review Comment: https://git.openjdk.org/jdk/pull/31059#discussion_r4352342899
