RFR: 8338383: Implement JEP 491: Synchronize Virtual Threads without Pinning

Wed Nov 6 17:39:47 UTC 2024

On Thu, 17 Oct 2024 14:28:30 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

> This is the implementation of JEP 491: Synchronize Virtual Threads without Pinning. See [JEP 491](https://bugs.openjdk.org/browse/JDK-8337395) for further details.
> 
> In order to make the code review easier the changes have been split into the following initial 4 commits:
> 
> - Changes to allow unmounting a virtual thread that is currently holding monitors.
> - Changes to allow unmounting a virtual thread blocked on synchronized trying to acquire the monitor.
> - Changes to allow unmounting a virtual thread blocked in `Object.wait()` and its timed-wait variants.
> - Changes to tests, JFR pinned event, and other changes in the JDK libraries.
> 
> The changes fix pinning issues for all 4 ports that currently implement continuations: x64, aarch64, riscv and ppc. Note: ppc changes were added recently and stand in its own commit after the initial ones.
> 
> The changes fix pinning issues when using `LM_LIGHTWEIGHT`, i.e. the default locking mode, (and `LM_MONITOR` which comes for free), but not when using `LM_LEGACY` mode. Note that the `LockingMode` flag has already been deprecated ([JDK-8334299](https://bugs.openjdk.org/browse/JDK-8334299)), with the intention to remove `LM_LEGACY` code in future releases.
> 
> 
> ## Summary of changes
> 
> ### Unmount virtual thread while holding monitors
> 
> As stated in the JEP, currently when a virtual thread enters a synchronized method or block, the JVM records the virtual thread's carrier platform thread as holding the monitor, not the virtual thread itself. This prevents the virtual thread from being unmounted from its carrier, as ownership information would otherwise go wrong. In order to fix this limitation we will do two things:
> 
> - We copy the oops stored in the LockStack of the carrier to the stackChunk when freezing (and clear the LockStack). We copy the oops back to the LockStack of the next carrier when thawing for the first time (and clear them from the stackChunk). Note that we currently assume carriers don't hold monitors while mounting virtual threads.
> 
> - For inflated monitors we now record the `java.lang.Thread.tid` of the owner in the ObjectMonitor's `_owner` field instead of a JavaThread*. This allows us to tie the owner of the monitor to a `java.lang.Thread` instance, rather than to a JavaThread which is only created per platform thread. The tid is already a 64 bit field so we can ignore issues of the counter wrapping around.
> 
> #### General notes about this part:
> 
> - Since virtual threads don't need to worry about holding monitors anymo...

First, congratulations on an exceptional piece of work @pchilano .  

Also thank you for the very clear breakdown and description in the PR as that helps immensely with trying to digest a change of this size.

The overall operational behaviour of this change seems very solid. My only concern is whether the unparker thread may become a bottleneck in some scenarios, but that is a bridge we will have to cross if we come to it.

My initial comments mainly come from just trying to understand the top-level changes around the use of the thread-id as the monitor owner. I have a number of suggestions on naming (mainly `is_x` versus `has_x`) and on documenting the API methods more clearly. None of which are showstoppers and some of which pre-exist. Unfortunately though you will need to fix the spelling of `succesor`.

Thanks

Thanks for those updates.

Thanks for updates. (I need to add a Review comment so I get a checkpoint to track further updates.)

Next batch of comments ...

Updates look good - thanks.

I think I have nothing further in terms of the review process. Great work!

Marked as reviewed by dholmes (Reviewer).

> The tid is cached in the JavaThread object under _lock_id. It is set on JavaThread creation and changed on mount/unmount.

Why do we need to cache it? Is it the implicit barriers related to accessing the `threadObj` oop each time?

Keeping this value up-to-date is a part I find quite confusing.

src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp line 2382:

> 2380:   __ bind(after_transition);
> 2381: 
> 2382:   if (LockingMode != LM_LEGACY && method->is_object_wait0()) {

It bothers me that we have to add a check for a specific native method in this code (notwithstanding there are already some checks in relation to hashCode). As a follow up I wonder if we can deal with wait-preemption by rewriting the Java code, instead of special casing the wait0 native code?

src/hotspot/share/classfile/javaClasses.cpp line 2082:

> 2080: }
> 2081: 
> 2082: bool java_lang_VirtualThread::set_onWaitingList(oop vthread, OopHandle& list_head) {

Some comments here about the operation would be useful. The "waiting list" here is just a list of virtual threads that need unparking by the Unblocker thread - right?

I'm struggling to understand how a thread can already be on this list?

src/hotspot/share/classfile/javaClasses.cpp line 2086:

> 2084:   jboolean vthread_on_list = Atomic::load(addr);
> 2085:   if (!vthread_on_list) {
> 2086:     vthread_on_list = Atomic::cmpxchg(addr, (jboolean)JNI_FALSE, (jboolean)JNI_TRUE);

It is not clear who the racing participants are here. How can the same thread be being placed on the list from two different actions?

src/hotspot/share/classfile/javaClasses.cpp line 2107:

> 2105: 
> 2106: jlong java_lang_VirtualThread::waitTimeout(oop vthread) {
> 2107:   return vthread->long_field(_timeout_offset);

Not sure what motivated the name change but it seems odd to have the method named differently to the field it accesses. ??

src/hotspot/share/code/nmethod.cpp line 711:

> 709:   // handle the case of an anchor explicitly set in continuation code that doesn't have a callee
> 710:   JavaThread* thread = reg_map->thread();
> 711:   if ((thread->has_last_Java_frame() && fr.sp() == thread->last_Java_sp()) JVMTI_ONLY(|| (method()->is_continuation_enter_intrinsic() && thread->on_monitor_waited_event()))) {

Suggestion:

  if ((thread->has_last_Java_frame() && fr.sp() == thread->last_Java_sp()) 
      JVMTI_ONLY(|| (method()->is_continuation_enter_intrinsic() && thread->on_monitor_waited_event()))) {

src/hotspot/share/prims/jvm.cpp line 4012:

> 4010:     }
> 4011:     ThreadBlockInVM tbivm(THREAD);
> 4012:     parkEvent->park();

What code does the unpark to wake this thread up? I can't quite see how this unparker thread operates as its logic seems dispersed.

src/hotspot/share/runtime/continuationFreezeThaw.cpp line 889:

> 887:     return f.is_native_frame() ? recurse_freeze_native_frame(f, caller) : recurse_freeze_stub_frame(f, caller);
> 888:   } else {
> 889:     // frame can't be freezed. Most likely the call_stub or upcall_stub

Suggestion:

    // Frame can't be frozen. Most likely the call_stub or upcall_stub

src/hotspot/share/runtime/javaThread.hpp line 165:

> 163:   // ID used as owner for inflated monitors. Same as the j.l.Thread.tid of the
> 164:   // current _vthread object, except during creation of the primordial and JNI
> 165:   // attached thread cases where this field can have a temporal value.

Suggestion:

  // attached thread cases where this field can have a temporary value.

Presumably this is for when the attaching thread is executing the Thread constructor?

src/hotspot/share/runtime/javaThread.hpp line 166:

> 164:   // current _vthread object, except during creation of the primordial and JNI
> 165:   // attached thread cases where this field can have a temporary value. Also,
> 166:   // calls to VirtualThread.switchToCarrierThread will temporary change _vthread

s/temporary change/temporarily change/

src/hotspot/share/runtime/objectMonitor.cpp line 132:

> 130: 
> 131: // -----------------------------------------------------------------------------
> 132: // Theory of operations -- Monitors lists, thread residency, etc:

This comment block needs updating now owner is not a JavaThread*, and to account for vthread usage

src/hotspot/share/runtime/objectMonitor.cpp line 1140:

> 1138: }
> 1139: 
> 1140: bool ObjectMonitor::resume_operation(JavaThread* current, ObjectWaiter* node, ContinuationWrapper& cont) {

Explanatory comment would be good - thanks.

src/hotspot/share/runtime/objectMonitor.cpp line 1532:

> 1530:   } else if (java_lang_VirtualThread::set_onWaitingList(vthread, vthread_cxq_head())) {
> 1531:     // Virtual thread case.
> 1532:     Trigger->unpark();

So ignoring for the moment that I can't see how `set_onWaitingList` could return false here, the check is just an optimisation to reduce the number of unparks issued i.e. only unpark if the list has changed?

src/hotspot/share/runtime/objectMonitor.cpp line 1673:

> 1671: 
> 1672:   ContinuationEntry* ce = current->last_continuation();
> 1673:   if (interruptible && ce != nullptr && ce->is_virtual_thread()) {

So IIUC this use of `interruptible` would be explained as follows:

// Some calls to wait() occur in contexts that still have to pin a vthread to its carrier.
// All such contexts perform non-interruptible waits, so by checking `interruptible` we know 
// this is a regular Object.wait call.

src/hotspot/share/runtime/objectMonitor.cpp line 1706:

> 1704:     // on _WaitSetLock so it's not profitable to reduce the length of the
> 1705:     // critical section.
> 1706: 

Please restore the blank line, else it looks like the comment block pertains to the `wait_reenter_begin`, but it doesn't.

src/hotspot/share/runtime/objectMonitor.cpp line 2028:

> 2026:   // First time we run after being preempted on Object.wait().
> 2027:   // Check if we were interrupted or the wait timed-out, and in
> 2028:   // that case remove ourselves from the _WaitSet queue.

I'm not sure how to interpret this comment block - is this really two sentences because the first is not actually a sentence. Also unclear what "run" and "First time" relate to.

src/hotspot/share/runtime/objectMonitor.cpp line 2054:

> 2052:   // Mark that we are at reenter so that we don't call this method again.
> 2053:   node->_at_reenter = true;
> 2054:   assert(!has_owner(current), "invariant");

The position of this assert seems odd as it seems to be something that should hold at entry to this method.

src/hotspot/share/runtime/objectMonitor.hpp line 47:

> 45: // ParkEvent instead.  Beware, however, that the JVMTI code
> 46: // knows about ObjectWaiters, so we'll have to reconcile that code.
> 47: // See next_waiter(), first_waiter(), etc.

This to-do is likely no longer relevant with the current changes.

src/hotspot/share/runtime/objectMonitor.hpp line 174:

> 172: 
> 173:   int64_t volatile _owner;  // Either tid of owner, NO_OWNER, ANONYMOUS_OWNER or DEFLATER_MARKER.
> 174:   volatile uint64_t _previous_owner_tid;  // thread id of the previous owner of the monitor

Looks odd to have the current owner as `int64_t` but we save the previous owner as `uint64_t`. ??

src/hotspot/share/runtime/objectMonitor.hpp line 207:

> 205: 
> 206:   static void Initialize();
> 207:   static void Initialize2();

Please add comment why this needs to be deferred - and till after what?

src/hotspot/share/runtime/objectMonitor.hpp line 288:

> 286:   // Returns true if this OM has an owner, false otherwise.
> 287:   bool      has_owner() const;
> 288:   int64_t   owner() const;  // Returns null if DEFLATER_MARKER is observed.

null is not an int64_t value.

src/hotspot/share/runtime/objectMonitor.hpp line 292:

> 290: 
> 291:   static int64_t owner_for(JavaThread* thread);
> 292:   static int64_t owner_for_oop(oop vthread);

Some comments describing this API would be good. I'm struggling a bit with the "owner for" terminology. I think `owner_from` would be better. And can't these just overload rather than using different names?

src/hotspot/share/runtime/objectMonitor.hpp line 299:

> 297:   // Simply set _owner field to new_value; current value must match old_value.
> 298:   void      set_owner_from_raw(int64_t old_value, int64_t new_value);
> 299:   // Same as above but uses tid of current as new value.

By `tid` here (and elsewhere) you actually mean `thread->threadObj()->thread_id()` - right?

src/hotspot/share/runtime/objectMonitor.hpp line 302:

> 300:   // Simply set _owner field to new_value; current value must match old_value.
> 301:   void      set_owner_from_raw(int64_t old_value, int64_t new_value);
> 302:   void      set_owner_from(int64_t old_value, JavaThread* current);

Again some comments describing API would good. The old API had vague names like old_value and new_value because of the different forms the owner value could take. Now it is always a thread-id we can do better I think. The distinction between the raw and non-raw forms is unclear and the latter is not covered by the initial comment.

src/hotspot/share/runtime/objectMonitor.hpp line 302:

> 300:   void      set_owner_from(int64_t old_value, JavaThread* current);
> 301:   // Set _owner field to tid of current thread; current value must be ANONYMOUS_OWNER.
> 302:   void      set_owner_from_BasicLock(JavaThread* current);

Shouldn't tid there be the basicLock?

src/hotspot/share/runtime/objectMonitor.hpp line 303:

> 301:   void      set_owner_from_raw(int64_t old_value, int64_t new_value);
> 302:   void      set_owner_from(int64_t old_value, JavaThread* current);
> 303:   // Simply set _owner field to current; current value must match basic_lock_p.

Comment is no longer accurate

src/hotspot/share/runtime/objectMonitor.hpp line 309:

> 307:   // _owner field. Returns the prior value of the _owner field.
> 308:   int64_t   try_set_owner_from_raw(int64_t old_value, int64_t new_value);
> 309:   int64_t   try_set_owner_from(int64_t old_value, JavaThread* current);

Similar to set_owner* need better comments describing API.

src/hotspot/share/runtime/objectMonitor.hpp line 311:

> 309:   int64_t   try_set_owner_from(int64_t old_value, JavaThread* current);
> 310: 
> 311:   bool      is_succesor(JavaThread* thread);

I think `has_successor` is more appropriate here as it is not the monitor that is the successor.

src/hotspot/share/runtime/objectMonitor.hpp line 312:

> 310:   void      set_successor(JavaThread* thread);
> 311:   void      set_successor(oop vthread);
> 312:   void      clear_successor();

Needs descriptive comments, or at least a preceding comment explaining what a "successor" is.

src/hotspot/share/runtime/objectMonitor.hpp line 315:

> 313:   void      set_succesor(oop vthread);
> 314:   void      clear_succesor();
> 315:   bool      has_succesor();

Sorry but `successor` has two `s` before `or`.

src/hotspot/share/runtime/objectMonitor.hpp line 317:

> 315:   bool      has_succesor();
> 316: 
> 317:   bool is_owner(JavaThread* thread) const { return owner() == owner_for(thread); }

Again `has_owner` seems more appropriate

src/hotspot/share/runtime/objectMonitor.hpp line 323:

> 321:   }
> 322: 
> 323:   bool is_owner_anonymous() const { return owner_raw() == ANONYMOUS_OWNER; }

Again I struggle with the pre-existing `is_owner` formulation here. The target of the expression is a monitor and we are asking if the monitor has an anonymous owner.

src/hotspot/share/runtime/objectMonitor.hpp line 333:

> 331:   bool is_stack_locker(JavaThread* current);
> 332:   BasicLock* stack_locker() const;
> 333:   void set_stack_locker(BasicLock* locker);

Again `is` versus `has`, plus some general comments describing the API.

src/hotspot/share/runtime/objectMonitor.hpp line 334:

> 332: 
> 333:   // Returns true if BasicLock* stored in _stack_locker
> 334:   // points to current's stack, false othwerwise.

Suggestion:

  // points to current's stack, false otherwise.

src/hotspot/share/runtime/objectMonitor.hpp line 349:

> 347:   ObjectWaiter* first_waiter()                                         { return _WaitSet; }
> 348:   ObjectWaiter* next_waiter(ObjectWaiter* o)                           { return o->_next; }
> 349:   JavaThread* thread_of_waiter(ObjectWaiter* o)                        { return o->_thread; }

This no longer looks correct if the waiter is a vthread. ??

src/hotspot/share/runtime/objectMonitor.inline.hpp line 110:

> 108: }
> 109: 
> 110: // Returns null if DEFLATER_MARKER is observed.

Comment needs updating

src/hotspot/share/runtime/objectMonitor.inline.hpp line 130:

> 128: // Returns true if owner field == DEFLATER_MARKER and false otherwise.
> 129: // This accessor is called when we really need to know if the owner
> 130: // field == DEFLATER_MARKER and any non-null value won't do the trick.

Comment needs updating

src/hotspot/share/runtime/synchronizer.cpp line 670:

> 668:   // Top native frames in the stack will not be seen if we attempt
> 669:   // preemption, since we start walking from the last Java anchor.
> 670:   NoPreemptMark npm(current);

Don't we still pin for JNI monitor usage?

src/hotspot/share/runtime/synchronizer.cpp line 1440:

> 1438: }
> 1439: 
> 1440: ObjectMonitor* ObjectSynchronizer::inflate_impl(JavaThread* inflating_thread, oop object, const InflateCause cause) {

`inflating_thread` doesn't sound right as it is always the current thread that is doing the inflating. The passed in thread may be a different thread trying to acquire the monitor ... perhaps `contending_thread`?

src/hotspot/share/runtime/synchronizer.hpp line 172:

> 170: 
> 171:   // Iterate ObjectMonitors where the owner is thread; this does NOT include
> 172:   // ObjectMonitors where owner is set to a stack lock address in thread.

Comment needs updating

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?

src/hotspot/share/services/threadService.cpp line 467:

> 465:         if (waitingToLockMonitor->has_owner()) {
> 466:           currentThread = Threads::owning_thread_from_monitor(t_list, waitingToLockMonitor);
> 467:           // If currentThread is nullptr we would like to know if the owner

Suggestion:

          // If currentThread is null we would like to know if the owner

src/hotspot/share/services/threadService.cpp line 474:

> 472:           // vthread we never record this as a deadlock. Note: unless there
> 473:           // is a bug in the VM, or a thread exits without releasing monitors
> 474:           // acquired through JNI, nullptr should imply unmounted vthread owner.

Suggestion:

          // acquired through JNI, null should imply an unmounted vthread owner.

src/java.base/share/classes/java/lang/Object.java line 383:

> 381:             try {
> 382:                 wait0(timeoutMillis);
> 383:             } catch (InterruptedException e) {

I had expected to see a call to a new `wait0` method that returned a value indicating whether the wait was completed or else we had to park. Instead we had to put special logic in the native-call-wrapper code in the VM to detect returning from wait0 and changing the return address. I'm still unclear where that modified return address actually takes us.

src/java.base/share/classes/java/lang/Thread.java line 654:

> 652:      * {@link Thread#PRIMORDIAL_TID} +1 as this class cannot be used during
> 653:      * early startup to generate the identifier for the primordial thread. The
> 654:      * counter is off-heap and shared with the VM to allow it assign thread

Suggestion:

     * counter is off-heap and shared with the VM to allow it to assign thread

src/java.base/share/classes/java/lang/Thread.java line 655:

> 653:      * early startup to generate the identifier for the primordial thread. The
> 654:      * counter is off-heap and shared with the VM to allow it assign thread
> 655:      * identifiers to non-Java threads.

Why do non-JavaThreads need an identifier of this kind?

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.

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?

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?

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?

src/java.base/share/classes/java/lang/VirtualThread.java line 115:

> 113:      *   RUNNING -> WAITING        // transitional state during wait on monitor
> 114:      *   WAITING -> WAITED         // waiting on monitor
> 115:      *    WAITED -> BLOCKED        // notified, waiting to be unblocked by monitor owner

Waiting to re-enter the monitor?

src/java.base/share/classes/java/lang/VirtualThread.java line 178:

> 176:     // timed-wait support
> 177:     private long waitTimeout;
> 178:     private byte timedWaitNonce;

Strange name - what does this mean?

src/java.base/share/classes/java/lang/VirtualThread.java line 530:

> 528:                 && carrier == Thread.currentCarrierThread();
> 529:         carrier.setCurrentThread(carrier);
> 530:         Thread.setCurrentLockId(this.threadId()); // keep lock ID of virtual thread

I'm struggling to understand the different threads in play when this is called and what the method actual does to which threads. ??

src/java.base/share/classes/java/lang/VirtualThread.java line 631:

> 629:         // Object.wait
> 630:         if (s == WAITING || s == TIMED_WAITING) {
> 631:             byte nonce;

Suggestion:

            byte seqNo;

src/java.base/share/classes/java/lang/VirtualThread.java line 948:

> 946:      * This method does nothing if the thread has been woken by notify or interrupt.
> 947:      */
> 948:     private void waitTimeoutExpired(byte nounce) {

I assume you meant `nonce` here, but please change to `seqNo`.

src/java.base/share/classes/java/lang/VirtualThread.java line 952:

> 950:         for (;;) {
> 951:             boolean unblocked = false;
> 952:             synchronized (timedWaitLock()) {

Where is the overall design of the timed-wait protocol and it use of synchronization described?

src/java.base/share/classes/java/lang/VirtualThread.java line 1397:

> 1395: 
> 1396:     /**
> 1397:      * Returns a lock object to coordinating timed-wait setup and timeout handling.

Suggestion:

     * Returns a lock object for coordinating timed-wait setup and timeout handling.

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

Changes requested by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21565#pullrequestreview-2384039238
PR Review: https://git.openjdk.org/jdk/pull/21565#pullrequestreview-2387241944
PR Review: https://git.openjdk.org/jdk/pull/21565#pullrequestreview-2393910702
PR Review: https://git.openjdk.org/jdk/pull/21565#pullrequestreview-2393922768
Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21565#pullrequestreview-2406338095
PR Review: https://git.openjdk.org/jdk/pull/21565#pullrequestreview-2409348761
PR Review: https://git.openjdk.org/jdk/pull/21565#pullrequestreview-2417279456
PR Comment: https://git.openjdk.org/jdk/pull/21565#issuecomment-2431004707
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1818251880
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815838204
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815839094
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1827128518
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815840245
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1814306675
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825344054
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1805616004
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1814260043
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815985700
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815998417
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816002660
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816009160
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816014286
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816017269
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816018848
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810025380
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815956322
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816040287
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810027786
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810029858
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1811912133
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810032387
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1811913172
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810033016
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810035434
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810037658
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815959203
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810036007
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810041017
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810046285
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810049295
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1811914377
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815960013
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815967260
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1815969101
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816043275
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816047142
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1816041444
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810068395
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825344940
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1825345446
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1814294622
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1814158735
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1814159210
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810076019
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810111255
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810113028
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810113953
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810114488
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810116177
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810131339
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1814169150
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1814170953
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1814171503
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1814172621