On Thu, 17 Oct 2024 14:28:30 GMT, Patricio Chilano Mateo <pchilanom...@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). 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
