On Tue, 25 Aug 2026 23:31:32 GMT, Serguei Spitsyn <[email protected]> wrote:
>> Shiv Shah has updated the pull request with a new target base due to a merge
>> or a rebase. The incremental webrev excludes the unrelated changes brought
>> in by the merge/rebase. The pull request contains three additional commits
>> since the last revision:
>>
>> - Require the Thread.sleep entry frame instead of accepting any JDK frame
>> - Merge branch 'master' into JDK-8340088-v2
>> - 8340088: Stack tracing tests of sleeping thread should be more resilient
>> to code changes
>
> test/hotspot/jtreg/vmTestbase/nsk/monitoring/share/ThreadController.java line
> 702:
>
>> 700: && elements[i].getMethodName().equals("sleep")) {
>> 701: return super.checkStackTrace(
>> 702: Arrays.copyOfRange(elements, i,
>> elements.length));
>
> Nit: A comment would be nice to have before the line 701 to explain why a
> call to the super.checkStackTrace() is needed. We already found a frame with
> the sleep() method. What else do we need here?
> Q: Also, Is it okay to have two methods with the same signature and
> implementation? Can we avoid this duplication?
The two frameworks sit in separate hierarchies with their own base checks so
there’s no shared spot for it without new cross package plumbing
> test/hotspot/jtreg/vmTestbase/nsk/monitoring/share/thread/SleepingThread.java
> line 105:
>
>> 103: if
>> (elements[i].getClassName().equals("java.lang.Thread")
>> 104: &&
>> elements[i].getMethodName().equals("sleep")) {
>> 105: return
>> super.checkStackTrace(Arrays.copyOfRange(elements, i, elements.length));
>
> Nit: A comment would be nice to have before the line 105 to explain why a
> call to the `super.checkStackTrace()` is needed. We already found a frame
> with the `sleep()` method. What else do we need here?
added
> test/hotspot/jtreg/vmTestbase/nsk/monitoring/stress/thread/strace001.java
> line 237:
>
>> 235: }
>> 236:
>> 237: // The method checks whether the element belongs to the test's own
>> code
>
> Nit: Need **dot** at the end of comment at line 237.
Added
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/31357#discussion_r3860125148
PR Review Comment: https://git.openjdk.org/jdk/pull/31357#discussion_r3860118772
PR Review Comment: https://git.openjdk.org/jdk/pull/31357#discussion_r3860117698
