On Tue, 20 Dec 2022 02:25:38 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>> There are a few nsk debugger tests that pin multiple virtual threads to >> carrier threads when synchronizing. Sometime the default number of carrier >> threads (which equals the number of CPUs) is not enough, and the test >> deadlocks because virtual threads start to wait forever for an available >> carrier thread. This PR fixes this problem by using the >> `jdk.virtualThreadScheduler.parallelism` property to change the default >> number of carrier threads. I believe the largest number of carrier threads >> any test needs is 11, so I chose 15 just to be safe. >> >> I had initially tried to fix each individual test by using the test support >> in `VThreadRunner.setParallism()`. The advantage of this was limiting the >> scope of the change to just a few tests, and also being able to specify the >> exact number of needed carrier threads. The disadvantage was having to make >> quite a few changes to quite a few tests, plus I had one troublesome test >> that was still failing, I believe because I didn't fully understand how many >> carrier threads it needed. Just giving every test 15 carrier threads in the >> end was a lot easier. > > Chris Plummer has updated the pull request incrementally with one additional > commit since the last revision: > > Better comments. > Are saying that in addition to the changes in this PR I should also change > each of the tests to add a check to make sure parallelism is set high enough? I was, but now I see the tests involved and the fact this problem is just an artifact of running those tests in virtual threads, then I really don't want to see those tests polluted with VT specific code. I know more now about the issue with pinning on monitor entry and not being able to increase parallelism for that case - but ideally that would indeed by the fix and I'll look into that some more. But this fix is approved. Thanks. ------------- Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.org/jdk/pull/11735
