On Sat, 13 Jun 2026 00:41:13 GMT, Leonid Mesnik <[email protected]> wrote:
>> The cur_stack_depth is the cache that is maintained in interponly mode to >> optimize getting number of thread in the debugger operations. >> >> Historically, it cause a lot of issues. Most oftenly, related with wrong >> calculation in the case if stack depth changed during yield or stack >> debugger operations. Also, there is a gep between decrement of >> cur_stack_depth and actual stack change where cur_stack_depth is invalid. >> >> The current problem happens when operations like PopFrame or >> ForceEarlyReturn happen when MethodExit is completed and cur_stack_depth is >> decremented while physical stack is not removed yet. It is reproduced by >> internal stress test. >> >> We discussed privately with @sspitsyn and there is an idea that this cache >> is not worth to maintain. It is used in interp-only mode which is slow >> already. Really this cur_stack_depth() is not so often used and quite often >> invalidated. >> >> Currently, the function is uses in 3 places: >> >> 1. JvmtiExport::continuation_yield_cleanup, where >> invalidate_cur_stack_depth() is called right before cur_stack_depth(), >> making the caching useless. So no performance degradation here is expected. >> 2. In the post_method_exit_inner when 'ets->has_frame_pops()' is true. And >> actually it might crash in this place. This happens when NotifyFramePop >> event is requested. It doesn't require the interponly mode after fix of >> [JDK-6960970](https://bugs.openjdk.org/browse/JDK-6960970) Debugger very >> slow during stepping >> 3. JvmtiThreadState::process_pending_step_for_popframe which is called in >> UpdateForPopTopFrameClosure during PopFrame when performance is not >> important. Also, the crash happens when 'process_pending_step_for_popframe' >> is executed while cache doesn't correspond the actual state. >> >> >> So I wonder if there are any scenario when the impact of the removal of >> cur_stack_depth is so significant that it makes sense to still maintain it? >> >> i am running tier1-8 and some internal stress testing to verify this fix. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Leonid Mesnik has updated the pull request incrementally with one additional > commit since the last revision: > > typo fixed Summary: I'm good with this version of the fix. Gory details of my analysis follow. The new version of the fix for 8383879: - adds one additional call to invalidate_cur_stack_depth() so we're now up to seven whack-a-mole calls from six; this new call is in the SetForceEarlyReturn::doit() function which makes perfect sense. - cur_stack_depth() is now called from two places instead of three. - count_frames() is now called from four places instead of three. JvmtiThreadState::cur_stack_depth() now goes slow-path (count_frames) instead of using the cache for two additional conditionals: is_pending_step_for_earlyret or is_pending_step_for_popframe. If we have an "early return" or a "pop frame" in process, it makes sense NOT to trust the cache. jvmtiThreadState::update_for_pop_top_frame() now goes slow-path (count_frames) instead of using the cache. Again, since we have a "pop frame" in process, it makes sense NOT to trust the cache. Here's an inventory of the places that call invalidate_cur_stack_depth(): src/hotspot/share/prims/jvmtiEnvBase.cpp: SetForceEarlyReturn::doit() - the new site; this makes sense since "force early return" is going to change the stack. src/hotspot/share/prims/jvmtiExport.cpp: JvmtiExport::continuation_yield_cleanup() - existing site; makes sense since a continuation yield will change the stack. src/hotspot/share/prims/jvmtiExport.cpp: JvmtiExport::post_exception_throw() - existing site; makes sense since throwing an exception will change the stack. src/hotspot/share/prims/jvmtiExport.cpp: JvmtiExport::notice_unwind_due_to_exception() - existing site; I was wondering if this is a duplicate call relative to post_exception_throw(), but it occurs to me that we would only call post_exception_throw() if that event is enabled. We have to "notice the unwind" whether or not we post an event. src/hotspot/share/prims/jvmtiThreadState.cpp: JvmtiThreadState::enter_interp_only_mode() - existing site; interp-only mode is where the cache CAN be good, but we have to flush an existing cache when we first get here. src/hotspot/share/prims/jvmtiThreadState.cpp: JvmtiThreadState::update_for_pop_top_frame() - existing site; makes sense since popping the top frame will change the stack. src/hotspot/share/runtime/continuationFreezeThaw.cpp: invalidate_jvmti_stack() - existing site; helper function so Loom can invalidate the cache when the continuation mechanism needs it done. The above is my attempt to rationalize the current set of whack-a-mole operations. ------------- Marked as reviewed by dcubed (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/31389#pullrequestreview-4510137866
