8369019: Improve timed-park mechanism in ObjectMonitor for virtual thread support #27597
Conversation
|
👋 Welcome back pchilanomate! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
/label remove core-libs |
|
@pchilano |
Webrevs
|
| // all other carriers have a vthread pinned to it waiting for said class | ||
| // to be loaded/initialized. | ||
| // If there are unmounted virtual threads ahead in the _entry_list we want | ||
| // to do a timed-park instead to alleviate some deadlocks cases where one |
There was a problem hiding this comment.
| // to do a timed-park instead to alleviate some deadlocks cases where one | |
| // to do a timed-park instead to alleviate some deadlock cases where one |
| assert_mark_word_consistency(); | ||
|
|
||
| // If there are unmounted virtual threads ahead in the _entry_list we want | ||
| // to do a timed-park instead to alleviate some deadlocks cases where one |
There was a problem hiding this comment.
| // to do a timed-park instead to alleviate some deadlocks cases where one | |
| // to do a timed-park instead to alleviate some deadlock cases where one |
| // _entry_list uses Atomic::cmpxchg() which already provides a fence that | ||
| // prevents this load from floating up previous store. | ||
| // Note that we can have false positives where timed-park is not necessary. | ||
| bool do_timed_parked = has_unmounted_vthreads(); |
There was a problem hiding this comment.
Don't we still only need the timed-park if the current thread is a pinned vthread?
There was a problem hiding this comment.
Yes, except if the monitor is also used in the context of a carrier thread. Currently there are only very few such cases and we disable preemption for them (e.g. interruptLock), so it’s likely not needed. With the upcoming changes to preempt on klass initialization, we could also have this situation if a class can be initialized both in the context of a carrier and a vthread. Since code executed in the context of the carriers is limited to library code there will also be very few cases of this, but I’ve seen at least one such case with LockSupport.
| // the notifier in notify_internal. | ||
| // Note that we can have false positives where timed-park is not necessary. | ||
| bool do_timed_parked = has_unmounted_vthreads(); | ||
| static int MAX_RECHECK_INTERVAL = 1000; |
There was a problem hiding this comment.
Is this a constant? This is the same as the enter case, should there be only one?
There was a problem hiding this comment.
Yes, I moved it to a global static.
Please review the following fix. When blocking in
ObjectMonitor::enter_internalwe currently use timed-park for pinned virtual threads. This is done to alleviate some potential deadlocks cases where the successor is an unmounted virtual thread that cannot run. In particular this could happen during class loading/initialization if all other carriers are blocked waiting for the same class to be loaded/initialized.This mechanism should be extended to cover
ObjectMonitor::reenter_internalused inObject.wait(notification case). Also, the criteria to decide whether to do a timed-park should be based on whether there are unmounted vthreads already in the_entry_list, and not just if this is a pinned virtual thread. This covers mixed usages of the same ObjectMonitor between virtual threads and platform threads. This will become more relevant once we bring the changes currently in the fibers branch to preempt virtual threads during klass initialization.These changes have been running in the loom pipeline for a couple of months already. I also added a new test case to test/jdk/java/lang/Thread/virtual/MonitorWaitNotify.java which deadlocks without these changes.
Thanks,
Patricio
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27597/head:pull/27597$ git checkout pull/27597Update a local copy of the PR:
$ git checkout pull/27597$ git pull https://git.openjdk.org/jdk.git pull/27597/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 27597View PR using the GUI difftool:
$ git pr show -t 27597Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27597.diff
Using Webrev
Link to Webrev Comment