Re: RFR: 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly [v4]
On Mon, 4 Mar 2024 09:51:32 GMT, Serguei Spitsyn wrote: >> src/hotspot/share/prims/jvmtiEnvBase.cpp line 941: >> >>> 939: bool is_virtual = java_lang_VirtualThread::is_instance(thread_oop); >>> 940: jint state = is_virtual ? >>> JvmtiEnvBase::get_vthread_state(thread_oop, java_thread) >>> 941: : >>> JvmtiEnvBase::get_thread_state(thread_oop, java_thread); >> >> I've seen this in a couple of your PRs now. It would be nice to have a >> utility function to get the JVMTI thread state of any java.lang.Thread >> instance. > > Good suggestion, thanks. Will add. Added now. - PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1512266627
Re: RFR: 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly [v4]
On Mon, 4 Mar 2024 09:49:07 GMT, Serguei Spitsyn wrote:
>> src/java.se/share/data/jdwp/jdwp.spec line 1985:
>>
>>> 1983: "thread may be waiting to enter the object's monitor, or in "
>>> 1984: "java.lang.Object.wait waiting to re-enter the monitor after
>>> being "
>>> 1985: "notified, interrupted, or timeout."
>>
>> `timed-out` would be the correct sense here.
>
> Thank you, David. I was thinking about it but was not sure. Will update it.
Fixed now. Also, updated CSR and RN.
>> src/jdk.jdi/share/classes/com/sun/jdi/ThreadReference.java line 314:
>>
>>> 312: * synchronized method, the synchronized statement, or
>>> 313: * {@link Object#wait} waiting to re-enter the monitor
>>> 314: * after being notified, interrupted, or timeout.
>>
>> Again `timed-out`.
>
> Will fix, thanks.
Fixed now. Also, updated CSR and RN.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1512112147
PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1512112210
Re: RFR: 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly [v4]
On Fri, 1 Mar 2024 11:19:21 GMT, Serguei Spitsyn wrote: >> The implementation of the JVM TI `GetCurrentContendedMonitor()` does not >> match the spec. It can sometimes return an incorrect information about the >> contended monitor. Such a behavior does not match the function spec. >> With this update the `GetCurrentContendedMonitor()` is returning the monitor >> only when the specified thread is waiting to enter or re-enter the monitor, >> and the monitor is not returned when the specified thread is waiting in the >> `java.lang.Object.wait` to be notified. >> >> The implementations of the JDWP `ThreadReference.CurrentContendedMonitor` >> command and JDI `ThreadReference.currentContendedMonitor()` method are based >> and depend on this JVMTI function. The JDWP command and the JDI method were >> specified incorrectly and had incorrect behavior. The fix slightly corrects >> both the JDWP and JDI specs. The JDWP and JDI implementations have been also >> fixed because they use this JVM TI update. Please, see and review the >> related CSR and Release-Note. >> >> CSR: [8326024](https://bugs.openjdk.org/browse/JDK-8326024): JVM TI >> GetCurrentContendedMonitor is implemented incorrectly >> RN: [8326038](https://bugs.openjdk.org/browse/JDK-8326038): Release Note: >> JVM TI GetCurrentContendedMonitor is implemented incorrectly >> >> Testing: >> - tested with the mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Merge > - Merge > - fix specs: JDWP ThreadReference.CurrentContendedMonitor command, JDI > ThreadReference.currentContendedMonitor method > - 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly David, thank you for review! - PR Comment: https://git.openjdk.org/jdk/pull/17944#issuecomment-1977937253
Re: RFR: 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly [v4]
On Mon, 4 Mar 2024 07:06:31 GMT, David Holmes wrote:
>> Serguei Spitsyn has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains four additional
>> commits since the last revision:
>>
>> - Merge
>> - Merge
>> - fix specs: JDWP ThreadReference.CurrentContendedMonitor command, JDI
>> ThreadReference.currentContendedMonitor method
>> - 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 941:
>
>> 939: bool is_virtual = java_lang_VirtualThread::is_instance(thread_oop);
>> 940: jint state = is_virtual ?
>> JvmtiEnvBase::get_vthread_state(thread_oop, java_thread)
>> 941: :
>> JvmtiEnvBase::get_thread_state(thread_oop, java_thread);
>
> I've seen this in a couple of your PRs now. It would be nice to have a
> utility function to get the JVMTI thread state of any java.lang.Thread
> instance.
Good suggestion, thanks. Will add.
> src/java.se/share/data/jdwp/jdwp.spec line 1985:
>
>> 1983: "thread may be waiting to enter the object's monitor, or in "
>> 1984: "java.lang.Object.wait waiting to re-enter the monitor after
>> being "
>> 1985: "notified, interrupted, or timeout."
>
> `timed-out` would be the correct sense here.
Thank you, David. I was thinking about it but was not sure. Will update it.
> src/jdk.jdi/share/classes/com/sun/jdi/ThreadReference.java line 314:
>
>> 312: * synchronized method, the synchronized statement, or
>> 313: * {@link Object#wait} waiting to re-enter the monitor
>> 314: * after being notified, interrupted, or timeout.
>
> Again `timed-out`.
Will fix, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1510890499
PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1510887118
PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1510887956
Re: RFR: 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly [v4]
On Fri, 1 Mar 2024 11:19:21 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetCurrentContendedMonitor()` does not
>> match the spec. It can sometimes return an incorrect information about the
>> contended monitor. Such a behavior does not match the function spec.
>> With this update the `GetCurrentContendedMonitor()` is returning the monitor
>> only when the specified thread is waiting to enter or re-enter the monitor,
>> and the monitor is not returned when the specified thread is waiting in the
>> `java.lang.Object.wait` to be notified.
>>
>> The implementations of the JDWP `ThreadReference.CurrentContendedMonitor`
>> command and JDI `ThreadReference.currentContendedMonitor()` method are based
>> and depend on this JVMTI function. The JDWP command and the JDI method were
>> specified incorrectly and had incorrect behavior. The fix slightly corrects
>> both the JDWP and JDI specs. The JDWP and JDI implementations have been also
>> fixed because they use this JVM TI update. Please, see and review the
>> related CSR and Release-Note.
>>
>> CSR: [8326024](https://bugs.openjdk.org/browse/JDK-8326024): JVM TI
>> GetCurrentContendedMonitor is implemented incorrectly
>> RN: [8326038](https://bugs.openjdk.org/browse/JDK-8326038): Release Note:
>> JVM TI GetCurrentContendedMonitor is implemented incorrectly
>>
>> Testing:
>> - tested with the mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains four additional
> commits since the last revision:
>
> - Merge
> - Merge
> - fix specs: JDWP ThreadReference.CurrentContendedMonitor command, JDI
> ThreadReference.currentContendedMonitor method
> - 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly
The main functional fix seems fine.
The JDWP and JDI spec changes seem fine - just a minor nit on wording.
I don't know enough about how the tests work to comment on those.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 941:
> 939: bool is_virtual = java_lang_VirtualThread::is_instance(thread_oop);
> 940: jint state = is_virtual ?
> JvmtiEnvBase::get_vthread_state(thread_oop, java_thread)
> 941: : JvmtiEnvBase::get_thread_state(thread_oop,
> java_thread);
I've seen this in a couple of your PRs now. It would be nice to have a utility
function to get the JVMTI thread state of any java.lang.Thread instance.
src/java.se/share/data/jdwp/jdwp.spec line 1985:
> 1983: "thread may be waiting to enter the object's monitor, or in "
> 1984: "java.lang.Object.wait waiting to re-enter the monitor after
> being "
> 1985: "notified, interrupted, or timeout."
`timed-out` would be the correct sense here.
src/jdk.jdi/share/classes/com/sun/jdi/ThreadReference.java line 314:
> 312: * synchronized method, the synchronized statement, or
> 313: * {@link Object#wait} waiting to re-enter the monitor
> 314: * after being notified, interrupted, or timeout.
Again `timed-out`.
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17944#pullrequestreview-1913493587
PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1510673055
PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1510665704
PR Review Comment: https://git.openjdk.org/jdk/pull/17944#discussion_r1510666362
Re: RFR: 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly [v4]
> The implementation of the JVM TI `GetCurrentContendedMonitor()` does not > match the spec. It can sometimes return an incorrect information about the > contended monitor. Such a behavior does not match the function spec. > With this update the `GetCurrentContendedMonitor()` is returning the monitor > only when the specified thread is waiting to enter or re-enter the monitor, > and the monitor is not returned when the specified thread is waiting in the > `java.lang.Object.wait` to be notified. > > The implementations of the JDWP `ThreadReference.CurrentContendedMonitor` > command and JDI `ThreadReference.currentContendedMonitor()` method are based > and depend on this JVMTI function. The JDWP command and the JDI method were > specified incorrectly and had incorrect behavior. The fix slightly corrects > both the JDWP and JDI specs. The JDWP and JDI implementations have been also > fixed because they use this JVM TI update. Please, see and review the related > CSR and Release-Note. > > CSR: [8326024](https://bugs.openjdk.org/browse/JDK-8326024): JVM TI > GetCurrentContendedMonitor is implemented incorrectly > RN: [8326038](https://bugs.openjdk.org/browse/JDK-8326038): Release Note: > JVM TI GetCurrentContendedMonitor is implemented incorrectly > > Testing: > - tested with the mach5 tiers 1-6 Serguei Spitsyn has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Merge - Merge - fix specs: JDWP ThreadReference.CurrentContendedMonitor command, JDI ThreadReference.currentContendedMonitor method - 8256314: JVM TI GetCurrentContendedMonitor is implemented incorrectly - Changes: - all: https://git.openjdk.org/jdk/pull/17944/files - new: https://git.openjdk.org/jdk/pull/17944/files/a4e8c5b5..1a3d1c19 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17944&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17944&range=02-03 Stats: 2399 lines in 780 files changed: 475 ins; 436 del; 1488 mod Patch: https://git.openjdk.org/jdk/pull/17944.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17944/head:pull/17944 PR: https://git.openjdk.org/jdk/pull/17944
