On Wed, 1 May 2024 19:02:29 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> address additional problem listed tests in hotspot/jtreg/serviceability > > test/hotspot/jtreg/serviceability/tmtools/jstack/DaemonThreadTest.java line > 77: > >> 75: t.setDaemon(false); >> 76: } >> 77: testThread(t, ""); > > I think the following is a bit clearer: > > > // Virtual threads are always daemon threads. Therefore if the current thread > is a > // virtual thread, then NormalThread will be a daemon thread and we need to > explicitly > // make it not a daemon thread. > > > You don't actually need the isVirtual() check, especially with the comment > explaining things. However, you could also forgo all this logic and the > comment by just sticking setDaemon(false) in the NormalThread constructor. Done - I've updated the PR to just call `setDaemon(false)` from the constructor of `NormalThread` instead of this conditional logic. When I initially added this logic, I was trying to avoid doing any changes that could impact the testing when running under platform threads. But thinking about this test's context, it merely wants to verify that the jstack output correctly presents daemon/non-daemon status, so explicitly setting a "normal" thread to non-daemon shouldn't impact what this test was originally verifying. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/19016#discussion_r1587033311
