RFR: 8345266: java/util/concurrent/locks/StampedLock/OOMEInStampedLock.java JTREG_TEST_THREAD_FACTORY=Virtual fails with OOME

[Coleen Phillimore]

On Tue, 17 Dec 2024 00:44:42 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

> Please review this small fix. The call to `Continuation::try_preempt()` can fail and return with a pending OOM exception due to insufficient memory while trying to allocate a new stackChunk. On this particular `OOMEInStampedLock.java` test, the OOM is thrown when returning from Object.wait, which is unexpected and causes the test to fail. But this could also happen when a virtual thread returns from monitorenter, which would cause an assert when coming from compiled code (see `SharedRuntime::monitor_enter_helper()`). Since a failing `try_preempt()` call will just prevent the vthread from being unmounted and force it to follow the normal platform thread code there is no need to throw OOM. The fix is to just clear the pending exception if set, and let future bytecodes handle the OOM condition if still present.
> 
> I verified that test `OOMEInStampedLock.java` now passes with the fix. I also added a new more specific test which fails without this fix and passes with it. I also run the change through mach5 tiers1-3.
> 
> Thanks,
> Patricio

Looks good!  Requesting a couple of comments.

src/hotspot/share/runtime/continuation.cpp line 165:

> 163: 
> 164:   if (target->has_pending_exception()) {
> 165:     assert(res == freeze_exception, "");

Can you add an assert string as additional documentation, something like "expecting an exception result from freeze";

src/hotspot/share/runtime/continuation.cpp line 167:

> 165:     assert(res == freeze_exception, "");
> 166:     // We don't want to throw exceptions, especially when returning
> 167:     // from monitorenter since the compiler does not expect one.

And add that ignoring the exception will propagate the failure and pin the virtual thread. Thanks for adding the comment for why this is okay.

test/jdk/java/lang/Thread/virtual/MonitorOOM.java line 85:

> 83:     }
> 84: 
> 85:     private static Object[] fillHeap() {

I'm surprised we don't have a helper class for eating memory, but I don't see one except in vmTestbase eatMemory which is more complicated for what you want here.

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

Marked as reviewed by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22780#pullrequestreview-2510184313
PR Review Comment: https://git.openjdk.org/jdk/pull/22780#discussion_r1889252214
PR Review Comment: https://git.openjdk.org/jdk/pull/22780#discussion_r1889253879
PR Review Comment: https://git.openjdk.org/jdk/pull/22780#discussion_r1889262598