RFR: 8335269: [Graal] occasional timeout in java/lang/StringBuffer/TestSynchronization.java with loom

[Alan Bateman]

On Wed, 3 Jul 2024 19:54:46 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

> Please review the following simple fix. A pinned virtual thread calling Thread.yield() in a loop might never poll for safepoints if the compiler relies on a poll in native method Continuation.doYield while optimizing. This is a special native method that doesn't always poll for safepoints, and in particular it doesn't if the virtual thread is pinned due to owning monitors. Currently this scenario can be reproduced with the Graal compiler.
> 
> I included a test which reproduces the issue with Graal (couldn't reproduce the issue with c2). The test times out without the fix and passes with it. I also run the patch through mach5 tiers1-3.
> 
> Thanks,
> Patricio

test/jdk/java/lang/Thread/virtual/ThreadYield.java line 29:

> 27:  * @summary Test that Thread.yield loop polls for safepoints
> 28:  * @requires vm.continuations
> 29:  * @modules java.base/java.lang:+open

I assume the `@modules` isn't needed as this test doesn't need to open java.lang.

test/jdk/java/lang/Thread/virtual/ThreadYield.java line 47:

> 45: import static org.junit.jupiter.api.Assertions.*;
> 46: 
> 47: class ThreadYield {

This isn't a unit test for Thread.yield so I think it would be better to rename to something specific like ThreadYieldPollsSafepoint (or better name).

test/jdk/java/lang/Thread/virtual/ThreadYield.java line 49:

> 47: class ThreadYield {
> 48:     static void foo(AtomicBoolean done) {
> 49:         synchronized (done) {

When this test makes it to the loom repo then we'll need to change it to pin by other means.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20016#discussion_r1668202884
PR Review Comment: https://git.openjdk.org/jdk/pull/20016#discussion_r1668202828
PR Review Comment: https://git.openjdk.org/jdk/pull/20016#discussion_r1668207758