RFR: 8338383: Implementation of Synchronize Virtual Threads without Pinning [v3]
David Holmes
dholmes at openjdk.org
Tue Oct 22 07:44:22 UTC 2024
On Tue, 22 Oct 2024 02:14:23 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 th...
>
> 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
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
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 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 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 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 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/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/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. ??
-------------
Changes requested by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/21565#pullrequestreview-2384039238
PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1810025380
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_r1810032387
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_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_r1810068395
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
More information about the nio-dev
mailing list