RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning [v3]

[Patricio Chilano Mateo]

On Tue, 22 Oct 2024 07:03:48 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Patricio Chilano Mateo has updated the pull request incrementally with six additional commits since the last revision:
>> 
>>  - Fix comments in objectMonitor.hpp
>>  - Move frame::saved_thread_address() to platform dependent files
>>  - Fix typo in jvmtiExport.cpp
>>  - remove usage of frame::metadata_words in possibly_adjust_frame()
>>  - Fix comments in c2 locking paths
>>  - Revert and simplify changes to c1_Runtime1 on aarch64 and riscv
>
> src/hotspot/share/runtime/threadIdentifier.cpp line 30:
> 
>> 28: 
>> 29: // starting at 3, excluding reserved values defined in ObjectMonitor.hpp
>> 30: static const int64_t INITIAL_TID = 3;
> 
> Can we express this in terms of those reserved values, or are they inaccessible?

Yes, we could define a new public constant `static const int64_t FIRST_AVAILABLE_TID = 3` (or some similar name) and use it here:


diff --git a/src/hotspot/share/runtime/threadIdentifier.cpp b/src/hotspot/share/runtime/threadIdentifier.cpp
index 60d6a990779..710c3141768 100644
--- a/src/hotspot/share/runtime/threadIdentifier.cpp
+++ b/src/hotspot/share/runtime/threadIdentifier.cpp
@@ -24,15 +24,15 @@

 #include "precompiled.hpp"
 #include "runtime/atomic.hpp"
+#include "runtime/objectMonitor.hpp"
 #include "runtime/threadIdentifier.hpp"

-// starting at 3, excluding reserved values defined in ObjectMonitor.hpp
-static const int64_t INITIAL_TID = 3;
-static volatile int64_t next_thread_id = INITIAL_TID;
+// excluding reserved values defined in ObjectMonitor.hpp
+static volatile int64_t next_thread_id = ObjectMonitor::FIRST_AVAILABLE_TID;

 #ifdef ASSERT
 int64_t ThreadIdentifier::initial() {
-  return INITIAL_TID;
+  return ObjectMonitor::FIRST_AVAILABLE_TID;
 }
 #endif

Or maybe define it as MAX_RESERVED_TID instead, and here we would add one to it.

> src/java.base/share/classes/java/lang/Thread.java line 731:
> 
>> 729: 
>> 730:         if (attached && VM.initLevel() < 1) {
>> 731:             this.tid = 3;  // primordial thread
> 
> The comment before the `ThreadIdentifiers` class needs updating to account for this change.

Fixed.

> src/java.base/share/classes/java/lang/VirtualThread.java line 109:
> 
>> 107:      *
>> 108:      *   RUNNING -> BLOCKING       // blocking on monitor enter
>> 109:      *  BLOCKING -> BLOCKED        // blocked on monitor enter
> 
> Should this say something similar to the parked case, about the "yield" being successful?

Since the unmount is triggered from the VM we never call yieldContinuation(), unlike with the PARKING case. In other words, there are no two cases to handle. If freezing the continuation fails, the virtual thread will already block in the monitor code pinned to the carrier, so a state of BLOCKING means freezing the continuation succeeded.

> src/java.base/share/classes/java/lang/VirtualThread.java line 110:
> 
>> 108:      *   RUNNING -> BLOCKING       // blocking on monitor enter
>> 109:      *  BLOCKING -> BLOCKED        // blocked on monitor enter
>> 110:      *   BLOCKED -> UNBLOCKED      // unblocked, may be scheduled to continue
> 
> Does this mean it now owns the monitor, or just it is able to re-contest for monitor entry?

It means it is scheduled to run again and re-contest for the monitor.

> src/java.base/share/classes/java/lang/VirtualThread.java line 111:
> 
>> 109:      *  BLOCKING -> BLOCKED        // blocked on monitor enter
>> 110:      *   BLOCKED -> UNBLOCKED      // unblocked, may be scheduled to continue
>> 111:      * UNBLOCKED -> RUNNING        // continue execution after blocked on monitor enter
> 
> Presumably this one means it acquired the monitor?

Not really, it is the state we set when the virtual thread is mounted and runs again. In this case it will just run to re-contest for the monitor.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1813237094
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1813237507
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1813239314
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1813239799
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1813240352