On Mon, 17 Jun 2024 09:13:57 GMT, Inigo Mediavilla Saiz <d...@openjdk.org> wrote:
> Follow up to https://github.com/openjdk/jdk/pull/19482 that was causing > issues when the PrintMountedVirtualTest.java was > running with `JTREG_TEST_THREAD_FACTORY=Virtual` in the loom repo. > > - Fixes issues where the test observes the thread during transitions. > - Fixes a potential issue in the test where CountDownLatch.countDown unparks > the main (virtual) thread and the main thread observes the dummy thread is > transition . src/hotspot/share/runtime/threads.cpp line 1334: > 1332: if (p->is_vthread_mounted()) { > 1333: const oop vt = p->vthread(); > 1334: if (vt != thread_oop) { We need a comment explaining why we have to check for this as without knowing the implementation quirks it seems non-sensical for a thread to have mounted itself! src/hotspot/share/runtime/threads.cpp line 1335: > 1333: const oop vt = p->vthread(); > 1334: if (vt != thread_oop) { > 1335: assert(vt != nullptr, "vthread should not be null when > vthread is mounted"); I think the assert still belongs in the original position doesn't it? Or could the problem we hit here cause a transient null to appear as well? test/hotspot/jtreg/serviceability/dcmd/thread/PrintMountedVirtualThread.java line 43: > 41: public void run(CommandExecutor executor) throws InterruptedException > { > 42: var shouldFinish = new AtomicBoolean(false); > 43: var started = new AtomicBoolean(); Not sure how this change make things better? Why would we want to sleep and poll rather than park until signalled? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19744#discussion_r1642745542 PR Review Comment: https://git.openjdk.org/jdk/pull/19744#discussion_r1642744456 PR Review Comment: https://git.openjdk.org/jdk/pull/19744#discussion_r1642747708
