On Fri, 6 Mar 2026 15:37:09 GMT, Markus Grönlund <[email protected]> wrote:
>> JFR Cooperative Sampling relies on a trick to reconstruct a sampled frame at >> the method exit poll instruction, because at that location, the frame has >> already been popped. The trick takes the frame_size() from the nmethod and >> subtracts that value from the SafepointBlob sender frames' sp, thus >> recreating the form of the just popped frame. >> >> With Valhalla's new scalarized and non-scalarized frames and calling >> conventions, this trick no longer works because the actual frame size is not >> kept in the nmethod; instead, it is part of the frame itself, on the stack >> (it's the first word below rbp). The problem for JFR Cooperative Sampling is >> that, at frame reconstruction time, the SafepointBlob stub will have >> overwritten the sp_inc slot of the popped frame, making frame reconstruction >> problematic and next to impossible. >> >> [JDK-8368099](https://bugs.openjdk.org/browse/JDK-8368099) provided a >> workaround for this problem by skipping all sampled frames with the property >> "needs_stack_repair" and moving directly to the sender frame instead. This >> results in biased sampling for nearly all samples taken inside frames whose >> next poll instruction is the method exit return. >> >> This solution handles both scalarized and non-scalarized frame layouts. >> >> Testing: jdk_valhalla, hotspot_valhalla, hotspot_valhalla_runtime, jdk_jfr, >> stress testing >> >> Thanks >> Markus > > Markus Grönlund has updated the pull request incrementally with one > additional commit since the last revision: > > normalized_bci Looks good to me. src/hotspot/share/jfr/recorder/stacktrace/jfrStackTrace.cpp line 213: > 211: assert(nm != nullptr, "invariant"); > 212: assert(nm->needs_stack_repair(), "invariant"); > 213: if (nm->is_native_method()) { I think this case should not be possible because we don't mark native methods with `needs_stack_repair`: https://github.com/openjdk/valhalla/blob/9024b8ee454e103248fc24cd95fd7543253600f4/src/hotspot/share/runtime/sharedRuntime.cpp#L2999 src/hotspot/share/jfr/recorder/stacktrace/jfrStackTrace.cpp line 227: > 225: > 226: static inline JfrSampleRequestFrameType frame_type(const > JfrSampleRequest& request) { > 227: const intptr_t value = p2i(request._sample_bcp); I see we have the following code when building the sampling request (also further down in `build_from_context`): https://github.com/openjdk/valhalla/blob/9024b8ee454e103248fc24cd95fd7543253600f4/src/hotspot/share/jfr/periodic/sampling/jfrSampleRequest.cpp#L133-L135 We are relying on this constant not matching `JfrSampleRequestFrameType::NEEDS_STACK_REPAIR`. Maybe we should use `JfrSampleRequestFrameType::INTERPRETER` now? test/jdk/jdk/jfr/event/profiling/TestNeedStackRepair.java line 41: > 39: /* > 40: * @test > 41: * @comment The purpose of this test is to verify that > jdk.ExecutionSample events taken inside non-scalarized frames Not sure whether there is established terminology for this, but referring to this c2 frame as "non-scalarized" is confusing to me since c2 uses the scalarized calling convention. The caller might be interpreted, but the arguments will still be unpacked at the entry point following the scalarized convention, and the return value will be returned as fields when possible. Why not simply refer to these as frames that need stack repair? (Note: frame extension could also happen with a c2 caller and a c1 callee) ------------- PR Review: https://git.openjdk.org/valhalla/pull/2176#pullrequestreview-3922297421 PR Review Comment: https://git.openjdk.org/valhalla/pull/2176#discussion_r2911653489 PR Review Comment: https://git.openjdk.org/valhalla/pull/2176#discussion_r2911656410 PR Review Comment: https://git.openjdk.org/valhalla/pull/2176#discussion_r2911663088
