On Thu, 11 Dec 2025 03:48:58 GMT, Leonid Mesnik <[email protected]> wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: use ReentrantLock.hasQueuedThread()
>
> test/hotspot/jtreg/serviceability/jvmti/vthread/ThreadListStackTracesTest/ThreadListStackTracesTest.java
> line 49:
>
>> 47: public void ensureReadyAndWaiting(Thread vt, Thread.State expState,
>> ReentrantLock rlock) {
>> 48: // wait while the thread is not ready or thread state is
>> unexpected
>> 49: while (!threadReady || (vt.getState() != expState) ||
>> !rlock.hasQueuedThreads()) {
>
> Small nit.
> The comment is incomplete. And code duplication is unneded.
>
> You can make
>
>
> task.ensureReady(vt, expState);
> // Ensure that thread waiting on the rlock
> while (!rlock.hasQueuedThreads()) {
> sleep(1);
> }
>
> to reduce code duplication.
>
> Really,
>
> task.ensureReady(vt, expState);
>
> Shouldn't be needed. We can't have lock.hasQueuedThreads() == true until
> we update `threadReady` and hit `vt.getState()`.
> But I think it is fine to check all conditions.
Thank you. I was thinking about the same but decided to go and introduce new
check method.
First, the check for `rlock.hasQueuedThreads()` seems to be good enough even
without `ensureReady()` call. But it is hard to be sure it is the case. Second,
the check to `ensureReady()` can be returned early without waiting on the
`rlock`. In such a case, we have to rely on the `rlock.hasQueuedThreads()`
which does not check the target thread state. Also, the code duplication is
small but it is more clean and reliable way to check the needed condition.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28734#discussion_r2609112411
