Re: RFR: 8297286: runtime/vthread tests crashing after JDK-8296324 [v2]
On Wed, 23 Nov 2022 10:14:23 GMT, Serguei Spitsyn wrote: >> This problem has two sides. >> One is that the `VirtualThread::run() `cashes the field `notifyJvmtiEvents` >> value. >> It caused the native method `notifyJvmtiUnmountBegin()` not called after the >> field `notifyJvmtiEvents` >> value has been set to `true` when an agent library is loaded into running VM. >> The fix is to get rid of this cashing. >> Another is that enabling `notifyJvmtiEvents` notifications needs a >> synchronization. >> Otherwise, a VTMS transition start can be missed which will cause some >> asserts to fire. >> The fix is to use a JvmtiVTMSTransitionDisabler helper for sync. >> >> Testing: >> The originally failed tests are passed now: >> >> runtime/vthread/RedefineClass.java >> runtime/vthread/TestObjectAllocationSampleEvent.java >> >> In progress: >> Run the tiers 1-6 to make sure there are no regression. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > remove caching if notifyJvmtiEvents in yieldContinuation Need to keep this PR alive for a while. - PR: https://git.openjdk.org/jdk/pull/11304
Re: RFR: 8297286: runtime/vthread tests crashing after JDK-8296324 [v2]
On Wed, 23 Nov 2022 10:14:23 GMT, Serguei Spitsyn wrote: >> This problem has two sides. >> One is that the `VirtualThread::run() `cashes the field `notifyJvmtiEvents` >> value. >> It caused the native method `notifyJvmtiUnmountBegin()` not called after the >> field `notifyJvmtiEvents` >> value has been set to `true` when an agent library is loaded into running VM. >> The fix is to get rid of this cashing. >> Another is that enabling `notifyJvmtiEvents` notifications needs a >> synchronization. >> Otherwise, a VTMS transition start can be missed which will cause some >> asserts to fire. >> The fix is to use a JvmtiVTMSTransitionDisabler helper for sync. >> >> Testing: >> The originally failed tests are passed now: >> >> runtime/vthread/RedefineClass.java >> runtime/vthread/TestObjectAllocationSampleEvent.java >> >> In progress: >> Run the tiers 1-6 to make sure there are no regression. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > remove caching if notifyJvmtiEvents in yieldContinuation One more artificial comment to keep the PR alive. - PR: https://git.openjdk.org/jdk/pull/11304
Re: RFR: 8297286: runtime/vthread tests crashing after JDK-8296324 [v2]
On Wed, 23 Nov 2022 10:14:23 GMT, Serguei Spitsyn wrote: >> This problem has two sides. >> One is that the `VirtualThread::run() `cashes the field `notifyJvmtiEvents` >> value. >> It caused the native method `notifyJvmtiUnmountBegin()` not called after the >> field `notifyJvmtiEvents` >> value has been set to `true` when an agent library is loaded into running VM. >> The fix is to get rid of this cashing. >> Another is that enabling `notifyJvmtiEvents` notifications needs a >> synchronization. >> Otherwise, a VTMS transition start can be missed which will cause some >> asserts to fire. >> The fix is to use a JvmtiVTMSTransitionDisabler helper for sync. >> >> Testing: >> The originally failed tests are passed now: >> >> runtime/vthread/RedefineClass.java >> runtime/vthread/TestObjectAllocationSampleEvent.java >> >> In progress: >> Run the tiers 1-6 to make sure there are no regression. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > remove caching if notifyJvmtiEvents in yieldContinuation This is artificial comment to keep the PR alive. - PR: https://git.openjdk.org/jdk/pull/11304
Re: RFR: 8297286: runtime/vthread tests crashing after JDK-8296324 [v2]
On Wed, 23 Nov 2022 10:14:23 GMT, Serguei Spitsyn wrote: >> This problem has two sides. >> One is that the `VirtualThread::run() `cashes the field `notifyJvmtiEvents` >> value. >> It caused the native method `notifyJvmtiUnmountBegin()` not called after the >> field `notifyJvmtiEvents` >> value has been set to `true` when an agent library is loaded into running VM. >> The fix is to get rid of this cashing. >> Another is that enabling `notifyJvmtiEvents` notifications needs a >> synchronization. >> Otherwise, a VTMS transition start can be missed which will cause some >> asserts to fire. >> The fix is to use a JvmtiVTMSTransitionDisabler helper for sync. >> >> Testing: >> The originally failed tests are passed now: >> >> runtime/vthread/RedefineClass.java >> runtime/vthread/TestObjectAllocationSampleEvent.java >> >> In progress: >> Run the tiers 1-6 to make sure there are no regression. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > remove caching if notifyJvmtiEvents in yieldContinuation I've forgotten the `JvmtiVTMSTransitionDisabler` is not going to work before the `notifyJvmtiEvents` is set to `true`. I agree, we may want to allow `start_VTMS_transition/finish_VTMS_transition` not properly paired as you suggest. But then it is not good that we loose the ability to strictly check/assert pairing of VTMS transition notifications for other cases. Need to think a bit more on this. - PR: https://git.openjdk.org/jdk/pull/11304
Re: RFR: 8297286: runtime/vthread tests crashing after JDK-8296324 [v2]
On Wed, 23 Nov 2022 10:14:23 GMT, Serguei Spitsyn wrote: >> This problem has two sides. >> One is that the `VirtualThread::run() `cashes the field `notifyJvmtiEvents` >> value. >> It caused the native method `notifyJvmtiUnmountBegin()` not called after the >> field `notifyJvmtiEvents` >> value has been set to `true` when an agent library is loaded into running VM. >> The fix is to get rid of this cashing. >> Another is that enabling `notifyJvmtiEvents` notifications needs a >> synchronization. >> Otherwise, a VTMS transition start can be missed which will cause some >> asserts to fire. >> The fix is to use a JvmtiVTMSTransitionDisabler helper for sync. >> >> Testing: >> The originally failed tests are passed now: >> >> runtime/vthread/RedefineClass.java >> runtime/vthread/TestObjectAllocationSampleEvent.java >> >> In progress: >> Run the tiers 1-6 to make sure there are no regression. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > remove caching if notifyJvmtiEvents in yieldContinuation Would it be possible to summarize behavior for when an agent enables the capability as a virtual thread executes for the first time or it continues after yield? More specifically JVMTI will be notified of a mount end without a correspond mount begin. It might be that we can narrow this down to if finish_VTMS_transition is okay without a preceding start_VTMS_transition. - PR: https://git.openjdk.org/jdk/pull/11304
Re: RFR: 8297286: runtime/vthread tests crashing after JDK-8296324 [v2]
On Wed, 23 Nov 2022 10:14:23 GMT, Serguei Spitsyn wrote: >> This problem has two sides. >> One is that the `VirtualThread::run() `cashes the field `notifyJvmtiEvents` >> value. >> It caused the native method `notifyJvmtiUnmountBegin()` not called after the >> field `notifyJvmtiEvents` >> value has been set to `true` when an agent library is loaded into running VM. >> The fix is to get rid of this cashing. >> Another is that enabling `notifyJvmtiEvents` notifications needs a >> synchronization. >> Otherwise, a VTMS transition start can be missed which will cause some >> asserts to fire. >> The fix is to use a JvmtiVTMSTransitionDisabler helper for sync. >> >> Testing: >> The originally failed tests are passed now: >> >> runtime/vthread/RedefineClass.java >> runtime/vthread/TestObjectAllocationSampleEvent.java >> >> In progress: >> Run the tiers 1-6 to make sure there are no regression. > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > remove caching if notifyJvmtiEvents in yieldContinuation Marked as reviewed by lmesnik (Reviewer). - PR: https://git.openjdk.org/jdk/pull/11304
Re: RFR: 8297286: runtime/vthread tests crashing after JDK-8296324 [v2]
On Wed, 23 Nov 2022 10:31:00 GMT, Alan Bateman wrote: >> Fixed the `yieldContinuation()` method. >> There is also `switchToCarrierThread()` method that returns the >> `notifyJvmtiEvents` value. >> It seems to be an optimization. I'm not sure yet, if we need to fix these >> places as well. > >> There is also `switchToCarrierThread()` method that returns the >> `notifyJvmtiEvents` value. >> It seems to be an optimization. I'm not sure yet, if we need to fix these >> places as well. > > It was to ensure that hide(true) and hide(false) are balanced. If it were to > re-poll notifyJvmtiEvents and a JVMTI agent enables the capability while a > thread in doing a temporary transition then you may get a hide(false) without > the corresponding hide(true). Okay, I see it now. - PR: https://git.openjdk.org/jdk/pull/11304
Re: RFR: 8297286: runtime/vthread tests crashing after JDK-8296324 [v2]
On Wed, 23 Nov 2022 10:16:44 GMT, Serguei Spitsyn wrote: > There is also `switchToCarrierThread()` method that returns the > `notifyJvmtiEvents` value. > It seems to be an optimization. I'm not sure yet, if we need to fix these > places as well. It was to ensure that hide(true) and hide(false) are balanced. If it were to re-poll notifyJvmtiEvents and a JVMTI agent enables the capability while a thread in doing a temporary transition then you may get a hide(false) without the corresponding hide(true). - PR: https://git.openjdk.org/jdk/pull/11304
Re: RFR: 8297286: runtime/vthread tests crashing after JDK-8296324 [v2]
On Wed, 23 Nov 2022 10:01:07 GMT, Serguei Spitsyn wrote:
>> src/java.base/share/classes/java/lang/VirtualThread.java line 273:
>>
>>> 271: private void run(Runnable task) {
>>> 272: assert state == RUNNING;
>>> 273: boolean notifyJvmti = notifyJvmtiEvents;
>>
>> Don't we have same issue in yieldContinuation() method? (line 396)
>
> Good point. I'll check and make an update if needed.
> Thank you for looking at it.
Fixed the `yieldContinuation()` method.
There is also `switchToCarrierThread()` method that returns the
`notifyJvmtiEvents` value.
It seems to be an optimization. I'm not sure yet, if we need to fix these
places as well.
-
PR: https://git.openjdk.org/jdk/pull/11304
Re: RFR: 8297286: runtime/vthread tests crashing after JDK-8296324 [v2]
> This problem has two sides. > One is that the `VirtualThread::run() `cashes the field `notifyJvmtiEvents` > value. > It caused the native method `notifyJvmtiUnmountBegin()` not called after the > field `notifyJvmtiEvents` > value has been set to `true` when an agent library is loaded into running VM. > The fix is to get rid of this cashing. > Another is that enabling `notifyJvmtiEvents` notifications needs a > synchronization. > Otherwise, a VTMS transition start can be missed which will cause some > asserts to fire. > The fix is to use a JvmtiVTMSTransitionDisabler helper for sync. > > Testing: > The originally failed tests are passed now: > > runtime/vthread/RedefineClass.java > runtime/vthread/TestObjectAllocationSampleEvent.java > > In progress: > Run the tiers 1-6 to make sure there are no regression. Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: remove caching if notifyJvmtiEvents in yieldContinuation - Changes: - all: https://git.openjdk.org/jdk/pull/11304/files - new: https://git.openjdk.org/jdk/pull/11304/files/c0d2f0ef..6608b1a6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=11304=01 - incr: https://webrevs.openjdk.org/?repo=jdk=11304=00-01 Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11304.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11304/head:pull/11304 PR: https://git.openjdk.org/jdk/pull/11304
