Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v26]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
review: removed incorrect spurious wakeup detection
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/94f30f16..effd0c17
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=25
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=24-25
Stats: 25 lines in 1 file changed: 1 ins; 24 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v25]
On Tue, 12 Mar 2024 05:13:54 GMT, David Holmes wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: addressed minor comments, updated a couple of copyright headers
>
> test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java
> line 352:
>
>> 350: wokeupCount++;
>> 351: if (!notifiedAll && notifiedCount < wokeupCount) {
>> 352: wasSpuriousWakeup = true;
>
> This doesn't work. First as you've realized you cannot detect a spurious
> wakeup when notifyAll is used. But second the `notifiedCount` is incremented
> on each `notify` whilst the monitor lock is still held - as it must be. So no
> `wait` can return until the monitor is unlocked. So unless the spurious
> wakeup occurs before the monitor is acquired at line 254, all notifications
> must be completed before even a spurious wakeup can cause `wait()` to return
> - at which point `notifiedCount` must equal ` NUMBER_OF_WAITING_THREADS`
> which can never be < `wokeupCount`.
> Given you can only detect one very specific case of a spurious wakeup, all
> this extra logic is just adding to the complexity and giving a false
> impression about what is being checked. To detect actual spurious wakeups
> here you need to be able to track when each thread was chosen by a `notify()`
> and there is no means to do that.
You are right, thanks!
Removed the incorrect spurious wakeup detection code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1521073727
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v25]
On Tue, 12 Mar 2024 02:31:43 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: addressed minor comments, updated a couple of copyright headers
Changes requested by dholmes (Reviewer).
test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java
line 352:
> 350: wokeupCount++;
> 351: if (!notifiedAll && notifiedCount < wokeupCount) {
> 352: wasSpuriousWakeup = true;
This doesn't work. First as you've realized you cannot detect a spurious wakeup
when notifyAll is used. But second the `notifiedCount` is incremented on each
`notify` whilst the monitor lock is still held - as it must be. So no `wait`
can return until the monitor is unlocked. So unless the spurious wakeup occurs
before the monitor is acquired at line 254, all notifications must be completed
before even a spurious wakeup can cause `wait()` to return - at which point
`notifiedCount` must equal ` NUMBER_OF_WAITING_THREADS` which can never be <
`wokeupCount`.
Given you can only detect one very specific case of a spurious wakeup, all this
extra logic is just adding to the complexity and giving a false impression
about what is being checked. To detect actual spurious wakeups here you need to
be able to track when each thread was chosen by a `notify()` and there is no
means to do that.
-
PR Review: https://git.openjdk.org/jdk/pull/17680#pullrequestreview-1930117927
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520862516
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v25]
On Tue, 12 Mar 2024 02:31:43 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: addressed minor comments, updated a couple of copyright headers
Thank you for review, Dan! I believe, all you comments have been addressed now.
-
PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1990070280
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v25]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
review: addressed minor comments, updated a couple of copyright headers
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/94ebf725..94f30f16
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=24
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=23-24
Stats: 13 lines in 6 files changed: 2 ins; 1 del; 10 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v23]
On Mon, 11 Mar 2024 18:23:25 GMT, Daniel D. Daugherty wrote: >> Serguei Spitsyn has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 26 commits: >> >> - Merge >> - review: minor tweak in test description of ObjectMonitorUsage.java >> - review: addressed more comments on the fix and new test >> - rename after merge: jvmti_common.h to jvmti_common.hpp >> - Merge >> - review: update comment in threads.hpp >> - fix deadlock with carrier threads starvation in ObjectMonitorUsage test >> - resolve merge conflict for deleted file objmonusage003.cpp >> - fix a typo in libObjectMonitorUsage.cpp >> - fix potential sync gap in the test ObjectMonitorUsage >> - ... and 16 more: https://git.openjdk.org/jdk/compare/de428daf...b97b8205 > > test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage001/TestDescription.java > line 46: > >> 44: * @library /vmTestbase >> 45: * /test/lib >> 46: * @run main/othervm/native -agentlib:objmonusage001=printdump >> nsk.jvmti.GetObjectMonitorUsage.objmonusage001 > > I only included this `=printdump` for debug when I was updating the test. > You don't have to keep it. Thanks, removed now. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520736552
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v23]
On Mon, 11 Mar 2024 18:44:34 GMT, Daniel D. Daugherty
wrote:
>> Serguei Spitsyn has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 26 commits:
>>
>> - Merge
>> - review: minor tweak in test description of ObjectMonitorUsage.java
>> - review: addressed more comments on the fix and new test
>> - rename after merge: jvmti_common.h to jvmti_common.hpp
>> - Merge
>> - review: update comment in threads.hpp
>> - fix deadlock with carrier threads starvation in ObjectMonitorUsage test
>> - resolve merge conflict for deleted file objmonusage003.cpp
>> - fix a typo in libObjectMonitorUsage.cpp
>> - fix potential sync gap in the test ObjectMonitorUsage
>> - ... and 16 more: https://git.openjdk.org/jdk/compare/de428daf...b97b8205
>
> test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/libObjectMonitorUsage.cpp
> line 215:
>
>> 213: if (tested_monitor != nullptr) {
>> 214: jni->DeleteGlobalRef(tested_monitor);
>> 215: }
>
> Since this is at the beginning of `setTestedMonitor` does it mean that we
> "leak"
> the last global ref?
Thank you for the check. There is no leak of the last global ref because there
is always this call at the end of each sub-test: `setTestedMonitor(null)`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520733090
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v23]
On Mon, 11 Mar 2024 18:43:11 GMT, Daniel D. Daugherty
wrote:
>> Serguei Spitsyn has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 26 commits:
>>
>> - Merge
>> - review: minor tweak in test description of ObjectMonitorUsage.java
>> - review: addressed more comments on the fix and new test
>> - rename after merge: jvmti_common.h to jvmti_common.hpp
>> - Merge
>> - review: update comment in threads.hpp
>> - fix deadlock with carrier threads starvation in ObjectMonitorUsage test
>> - resolve merge conflict for deleted file objmonusage003.cpp
>> - fix a typo in libObjectMonitorUsage.cpp
>> - fix potential sync gap in the test ObjectMonitorUsage
>> - ... and 16 more: https://git.openjdk.org/jdk/compare/de428daf...b97b8205
>
> test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/libObjectMonitorUsage.cpp
> line 202:
>
>> 200: LOG("(%d) notify_waiter_count expected: %d, actually: %d\n",
>> 201: check_idx, notifyWaiterCount, inf.notify_waiter_count);
>> 202: result = STATUS_FAILED;
>
> Please consider prefixing these LOG output lines with "FAILED:" so that it
> is easier to identify the log output where failure info is shown.
Good suggestion. Fixed now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520726685
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v21]
On Mon, 11 Mar 2024 18:38:52 GMT, Daniel D. Daugherty wrote: >> Okay, thanks! I'll add some diagnostic code to catch spurious wakups. >> Let's see if we ever encounter any spurious wakeup in this test. > > I think some sort of comment needs to be added here to document > the possibility of this code path being affect by a spurious wakeup. Thanks. I've added both a comment and also some spurious wakeup detection with a warning. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520719450
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v24]
On Mon, 11 Mar 2024 18:14:49 GMT, Daniel D. Daugherty wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> improved new test: added wakeup warning; polished test ouput > > test/hotspot/jtreg/TEST.quick-groups line 972: > >> 970: >> vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage001/TestDescription.java >> \ >> 971: >> vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage002/TestDescription.java >> \ >> 972: >> vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage003/TestDescription.java >> \ > > This file needs a copyright year update. Thanks, fixed now. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520723644
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v22]
On Mon, 11 Mar 2024 01:53:09 GMT, David Holmes wrote: >> Okay. Now I see why the old code did not have endless loops. > > Apologies I should have checked the code - I'd forgotten the list is circular > to avoid needing an explicit tail pointer. Okay, thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520719906
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v23]
On Mon, 11 Mar 2024 18:05:00 GMT, Daniel D. Daugherty wrote: >> Serguei Spitsyn has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 26 commits: >> >> - Merge >> - review: minor tweak in test description of ObjectMonitorUsage.java >> - review: addressed more comments on the fix and new test >> - rename after merge: jvmti_common.h to jvmti_common.hpp >> - Merge >> - review: update comment in threads.hpp >> - fix deadlock with carrier threads starvation in ObjectMonitorUsage test >> - resolve merge conflict for deleted file objmonusage003.cpp >> - fix a typo in libObjectMonitorUsage.cpp >> - fix potential sync gap in the test ObjectMonitorUsage >> - ... and 16 more: https://git.openjdk.org/jdk/compare/de428daf...b97b8205 > > src/hotspot/share/prims/jvmtiEnvBase.cpp line 1483: > >> 1481: markWord mark = hobj->mark(); >> 1482: ResourceMark rm(current_thread); >> 1483: GrowableArray* wantList = nullptr; > > Thanks for refactoring `JvmtiEnvBase::get_object_monitor_usage()` to > be much more clear. It was difficult to review in GitHub so I switched to > the webrev frames view so that I could scroll the two sides back and forth > and determine equivalence. Good! > src/hotspot/share/runtime/threads.hpp line 134: > >> 132: static unsigned print_threads_compiling(outputStream* st, char* buf, >> int buflen, bool short_form = false); >> 133: >> 134: // Get count Java threads that are waiting to enter or re-enter the >> specified monitor. > > nit typo: s/count Java/count of Java/ > > Also this file needs a copyright year update. Thanks, fixed now. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520721501 PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520722461
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v23]
On Mon, 11 Mar 2024 18:08:01 GMT, Daniel D. Daugherty wrote: >> Serguei Spitsyn has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 26 commits: >> >> - Merge >> - review: minor tweak in test description of ObjectMonitorUsage.java >> - review: addressed more comments on the fix and new test >> - rename after merge: jvmti_common.h to jvmti_common.hpp >> - Merge >> - review: update comment in threads.hpp >> - fix deadlock with carrier threads starvation in ObjectMonitorUsage test >> - resolve merge conflict for deleted file objmonusage003.cpp >> - fix a typo in libObjectMonitorUsage.cpp >> - fix potential sync gap in the test ObjectMonitorUsage >> - ... and 16 more: https://git.openjdk.org/jdk/compare/de428daf...b97b8205 > > src/hotspot/share/runtime/threads.cpp line 1186: > >> 1184: >> 1185: #if INCLUDE_JVMTI >> 1186: // Get count Java threads that are waiting to enter or re-enter the >> specified monitor. > > nit typo: s/count Java/count of Java/ Thanks, fixed now. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520714780
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v24]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
improved new test: added wakeup warning; polished test ouput
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/b97b8205..94ebf725
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=23
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=22-23
Stats: 61 lines in 2 files changed: 45 ins; 4 del; 12 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v23]
On Fri, 8 Mar 2024 06:11:23 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 26 commits:
>
> - Merge
> - review: minor tweak in test description of ObjectMonitorUsage.java
> - review: addressed more comments on the fix and new test
> - rename after merge: jvmti_common.h to jvmti_common.hpp
> - Merge
> - review: update comment in threads.hpp
> - fix deadlock with carrier threads starvation in ObjectMonitorUsage test
> - resolve merge conflict for deleted file objmonusage003.cpp
> - fix a typo in libObjectMonitorUsage.cpp
> - fix potential sync gap in the test ObjectMonitorUsage
> - ... and 16 more: https://git.openjdk.org/jdk/compare/de428daf...b97b8205
Thumbs up on the v22 version of this change. Of course, I'm assuming
that the updated tests pass and that there are no new failures in the
usual Mach5 testing.
Sorry for the delay in getting back to this review. A combination of
vacation following by illness delayed me getting back to this PR.
Please note that I did not try to compare the (deleted) objmonusage003
version of the test with the (new) ObjectMonitorUsage version. I didn't
think that would be useful.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1483:
> 1481: markWord mark = hobj->mark();
> 1482: ResourceMark rm(current_thread);
> 1483: GrowableArray* wantList = nullptr;
Thanks for refactoring `JvmtiEnvBase::get_object_monitor_usage()` to
be much more clear. It was difficult to review in GitHub so I switched to
the webrev frames view so that I could scroll the two sides back and forth
and determine equivalence.
src/hotspot/share/runtime/threads.cpp line 1186:
> 1184:
> 1185: #if INCLUDE_JVMTI
> 1186: // Get count Java threads that are waiting to enter or re-enter the
> specified monitor.
nit typo: s/count Java/count of Java/
src/hotspot/share/runtime/threads.hpp line 134:
> 132: static unsigned print_threads_compiling(outputStream* st, char* buf,
> int buflen, bool short_form = false);
> 133:
> 134: // Get count Java threads that are waiting to enter or re-enter the
> specified monitor.
nit typo: s/count Java/count of Java/
Also this file needs a copyright year update.
test/hotspot/jtreg/TEST.quick-groups line 972:
> 970:
> vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage001/TestDescription.java
> \
> 971:
>
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v21]
On Mon, 11 Mar 2024 18:08:14 GMT, Serguei Spitsyn wrote: >> Sorry I missed this response. I can't see a way to address spurious wakeups >> in this case as it needs to be a per-thread flag (so that each thread knows >> it was notified) but you don't know which thread will be notified in any >> given call to `notify()`. I also can't see how you can detect a spurious >> wakeup in this code. If they happen then a subtest may fail due to an >> unexpected number of re-entering threads. >> I think we will just have to see how stable the test is in practice. > > Okay, thanks! I'll add some diagnostic code to catch spurious wakups. > Let's see if we ever encounter any spurious wakeup in this test. I think some sort of comment needs to be added here to document the possibility of this code path being affect by a spurious wakeup. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520260059
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v21]
On Mon, 11 Mar 2024 02:18:57 GMT, David Holmes wrote: >> I have an idea how to fix it but it will add extra complexity. Not sure it >> is worth it. >> It is possible to identify there was a spurious wakeup and invalidate the >> sub-test result in such a case. >> Not sure if it is important to rerun the sub-test then. >> What do you think? > > Sorry I missed this response. I can't see a way to address spurious wakeups > in this case as it needs to be a per-thread flag (so that each thread knows > it was notified) but you don't know which thread will be notified in any > given call to `notify()`. I also can't see how you can detect a spurious > wakeup in this code. If they happen then a subtest may fail due to an > unexpected number of re-entering threads. > I think we will just have to see how stable the test is in practice. Okay, thanks! I'll add some diagnostic code to catch spurious wakups. Let's see if we ever encounter any spurious wakeup in this test. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520204046
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v23]
On Fri, 8 Mar 2024 06:11:23 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 26 commits:
>
> - Merge
> - review: minor tweak in test description of ObjectMonitorUsage.java
> - review: addressed more comments on the fix and new test
> - rename after merge: jvmti_common.h to jvmti_common.hpp
> - Merge
> - review: update comment in threads.hpp
> - fix deadlock with carrier threads starvation in ObjectMonitorUsage test
> - resolve merge conflict for deleted file objmonusage003.cpp
> - fix a typo in libObjectMonitorUsage.cpp
> - fix potential sync gap in the test ObjectMonitorUsage
> - ... and 16 more: https://git.openjdk.org/jdk/compare/de428daf...b97b8205
I last reviewed v02 of this PR so I've jumped forward to the full v22 webrev.
It may take
me a bit of time to redo this review.
-
PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1989046765
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v23]
On Fri, 8 Mar 2024 06:11:23 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 26 commits:
>
> - Merge
> - review: minor tweak in test description of ObjectMonitorUsage.java
> - review: addressed more comments on the fix and new test
> - rename after merge: jvmti_common.h to jvmti_common.hpp
> - Merge
> - review: update comment in threads.hpp
> - fix deadlock with carrier threads starvation in ObjectMonitorUsage test
> - resolve merge conflict for deleted file objmonusage003.cpp
> - fix a typo in libObjectMonitorUsage.cpp
> - fix potential sync gap in the test ObjectMonitorUsage
> - ... and 16 more: https://git.openjdk.org/jdk/compare/de428daf...b97b8205
> Sorry I missed this response. I can't see a way to address spurious wakeups
> in this case as it needs to be a per-thread flag (so that each thread knows
> it was notified) but you don't know which thread will be notified in any
> given call to notify(). I also can't see how you can detect a spurious wakeup
> in this code. If they happen then a subtest may fail due to an unexpected
> number of re-entering threads. I think we will just have to see how stable
> the test is in practice.
Okay, thanks! Let's see if we ever encounter any spurious wakeup in this test.
Than you a lot for thorough review, David!
-
PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1988074775
PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1988076636
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v21]
On Mon, 4 Mar 2024 10:09:10 GMT, Serguei Spitsyn wrote:
>> test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java
>> line 319:
>>
>>> 317: try {
>>> 318: ready = true;
>>> 319: lockCheck.wait();
>>
>> Spurious wakeups will break this test. They shouldn't happen but ...
>
> I have an idea how to fix it but it will add extra complexity. Not sure it is
> worth it.
> It is possible to identify there was a spurious wakeup and invalidate the
> sub-test result in such a case.
> Not sure if it is important to rerun the sub-test then.
> What do you think?
Sorry I missed this response. I can't see a way to address spurious wakeups in
this case as it needs to be a per-thread flag (so that each thread knows it was
notified) but you don't know which thread will be notified in any given call to
`notify()`. I also can't see how you can detect a spurious wakeup in this code.
If they happen then a subtest may fail due to an unexpected number of
re-entering threads.
I think we will just have to see how stable the test is in practice.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1519071056
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v22]
On Tue, 5 Mar 2024 09:04:08 GMT, Serguei Spitsyn wrote:
>> The loop is endless without this extra condition, so we are getting a test
>> execution timeout.
>> The `waiters` seems to be `circular doubly linked list` as we can see below:
>>
>> inline void ObjectMonitor::AddWaiter(ObjectWaiter* node) {
>> assert(node != nullptr, "should not add null node");
>> assert(node->_prev == nullptr, "node already in list");
>> assert(node->_next == nullptr, "node already in list");
>> // put node at end of queue (circular doubly linked list)
>> if (_WaitSet == nullptr) {
>> _WaitSet = node;
>> node->_prev = node;
>> node->_next = node;
>> } else {
>> ObjectWaiter* head = _WaitSet;
>> ObjectWaiter* tail = head->_prev;
>> assert(tail->_next == head, "invariant check");
>> tail->_next = node;
>> head->_prev = node;
>> node->_next = head;
>> node->_prev = tail;
>> }
>> }
>>
>> I'll make sure nothing is missed and check the old code again.
>
> Okay. Now I see why the old code did not have endless loops.
Apologies I should have checked the code - I'd forgotten the list is circular
to avoid needing an explicit tail pointer.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1519058800
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v23]
On Fri, 8 Mar 2024 06:11:23 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 26 commits:
>
> - Merge
> - review: minor tweak in test description of ObjectMonitorUsage.java
> - review: addressed more comments on the fix and new test
> - rename after merge: jvmti_common.h to jvmti_common.hpp
> - Merge
> - review: update comment in threads.hpp
> - fix deadlock with carrier threads starvation in ObjectMonitorUsage test
> - resolve merge conflict for deleted file objmonusage003.cpp
> - fix a typo in libObjectMonitorUsage.cpp
> - fix potential sync gap in the test ObjectMonitorUsage
> - ... and 16 more: https://git.openjdk.org/jdk/compare/de428daf...b97b8205
Looks good. Thanks.
Sorry it has taken me so long to work through this one. It seemed so simple
when I filed the bug :)
-
Marked as reviewed by dholmes (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17680#pullrequestreview-1926797746
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v23]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 26 commits:
- Merge
- review: minor tweak in test description of ObjectMonitorUsage.java
- review: addressed more comments on the fix and new test
- rename after merge: jvmti_common.h to jvmti_common.hpp
- Merge
- review: update comment in threads.hpp
- fix deadlock with carrier threads starvation in ObjectMonitorUsage test
- resolve merge conflict for deleted file objmonusage003.cpp
- fix a typo in libObjectMonitorUsage.cpp
- fix potential sync gap in the test ObjectMonitorUsage
- ... and 16 more: https://git.openjdk.org/jdk/compare/de428daf...b97b8205
-
Changes: https://git.openjdk.org/jdk/pull/17680/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=17680=22
Stats: 1161 lines in 15 files changed: 722 ins; 390 del; 49 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v22]
On Tue, 5 Mar 2024 08:53:42 GMT, Serguei Spitsyn wrote:
>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1507:
>>
>>> 1505: nWait = 0;
>>> 1506: for (ObjectWaiter* waiter = mon->first_waiter();
>>> 1507: waiter != nullptr && (nWait == 0 || waiter !=
>>> mon->first_waiter());
>>
>> Sorry I do not understand the logic of this line. `waiters` is just a
>> linked-list of `ObjectWaiter`s so all we need to do is traverse it and count
>> the number of elements.
>
> The loop is endless without this extra condition, so we are getting a test
> execution timeout.
> The `waiters` seems to be `circular doubly linked list` as we can see below:
>
> inline void ObjectMonitor::AddWaiter(ObjectWaiter* node) {
> assert(node != nullptr, "should not add null node");
> assert(node->_prev == nullptr, "node already in list");
> assert(node->_next == nullptr, "node already in list");
> // put node at end of queue (circular doubly linked list)
> if (_WaitSet == nullptr) {
> _WaitSet = node;
> node->_prev = node;
> node->_next = node;
> } else {
> ObjectWaiter* head = _WaitSet;
> ObjectWaiter* tail = head->_prev;
> assert(tail->_next == head, "invariant check");
> tail->_next = node;
> head->_prev = node;
> node->_next = head;
> node->_prev = tail;
> }
> }
>
> I'll make sure nothing is missed and check the old code again.
Okay. Now I see why the old code did not have endless loops.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1512409266
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v22]
On Tue, 5 Mar 2024 06:30:20 GMT, David Holmes wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: addressed more comments on the fix and new test
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1507:
>
>> 1505: nWait = 0;
>> 1506: for (ObjectWaiter* waiter = mon->first_waiter();
>> 1507: waiter != nullptr && (nWait == 0 || waiter !=
>> mon->first_waiter());
>
> Sorry I do not understand the logic of this line. `waiters` is just a
> linked-list of `ObjectWaiter`s so all we need to do is traverse it and count
> the number of elements.
The loop is endless without this extra condition, so we are getting a test
execution timeout.
The `waiters` seems to be `circular doubly linked list` as we can see below:
inline void ObjectMonitor::AddWaiter(ObjectWaiter* node) {
assert(node != nullptr, "should not add null node");
assert(node->_prev == nullptr, "node already in list");
assert(node->_next == nullptr, "node already in list");
// put node at end of queue (circular doubly linked list)
if (_WaitSet == nullptr) {
_WaitSet = node;
node->_prev = node;
node->_next = node;
} else {
ObjectWaiter* head = _WaitSet;
ObjectWaiter* tail = head->_prev;
assert(tail->_next == head, "invariant check");
tail->_next = node;
head->_prev = node;
node->_next = head;
node->_prev = tail;
}
}
I'll make sure nothing is missed and check the old code again.
> test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java
> line 36:
>
>> 34: * - unowned object without any waiting threads
>> 35: * - unowned object with threads waiting to be notified
>> 36: * - owned object without any waiting threads
>
> You could say "threads waiting" in all cases - it looks odd to reverse it for
> two cases.
Thanks, updated.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1512386413
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1512391923
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v22]
On Tue, 5 Mar 2024 03:40:04 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: addressed more comments on the fix and new test
Updates look good. A couple of minor commenst.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1507:
> 1505: nWait = 0;
> 1506: for (ObjectWaiter* waiter = mon->first_waiter();
> 1507: waiter != nullptr && (nWait == 0 || waiter !=
> mon->first_waiter());
Sorry I do not understand the logic of this line. `waiters` is just a
linked-list of `ObjectWaiter`s so all we need to do is traverse it and count
the number of elements.
test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java
line 36:
> 34: * - unowned object without any waiting threads
> 35: * - unowned object with threads waiting to be notified
> 36: * - owned object without any waiting threads
You could say "threads waiting" in all cases - it looks odd to reverse it for
two cases.
-
PR Review: https://git.openjdk.org/jdk/pull/17680#pullrequestreview-1915963487
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1512195706
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1512197846
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v22]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
review: addressed more comments on the fix and new test
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/b449f04a..ccf9484f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=21
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=20-21
Stats: 69 lines in 3 files changed: 39 ins; 4 del; 26 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v21]
On Mon, 4 Mar 2024 10:02:03 GMT, Serguei Spitsyn wrote: >> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1552: >> >>> 1550: // If the thread was found on the ObjectWaiter list, then >>> 1551: // it has not been notified. This thread can't change the >>> 1552: // state of the monitor so it doesn't need to be suspended. >> >> Not sure why suspension is being mentioned here. > > This is the original comment. I also do not quite understand it. It is > confusing for sure. I can remove it if Dan does not object. Removed this statement from the comment. Will restore in a case of Dan's objection. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1512020517
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v21]
On Mon, 4 Mar 2024 03:28:28 GMT, David Holmes wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> rename after merge: jvmti_common.h to jvmti_common.hpp > > test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java > line 40: > >> 38: * - owned object with N waitings to be notified >> 39: * - owned object with N waitings to enter, from 0 to N waitings >> to re-enter, >> 40: * from N to 0 waitings to be notified > > "waitings" is not grammatically valid. Suggestion: s/waitings/threads waiting/ Thanks, fixed now. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510976777
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v21]
On Mon, 4 Mar 2024 03:26:58 GMT, David Holmes wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> rename after merge: jvmti_common.h to jvmti_common.hpp > > test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java > line 31: > >> 29: * DESCRIPTION >> 30: * The test checks if the JVMTI function GetObjectMonitorUsage >> returns >> 31: * the expected values for the owner, entry_count, water_count > > s/water_count/waiter_count/ Thanks, fixed now. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510947433
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v21]
On Mon, 4 Mar 2024 03:29:01 GMT, David Holmes wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> rename after merge: jvmti_common.h to jvmti_common.hpp > > test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java > line 42: > >> 40: * from N to 0 waitings to be notified >> 41: * - all the above scenarios are executed with platform and >> virtual threads >> 42: * @requires vm.continuations > > Do we really require this? Yes, can be removed. Thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510925088
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v21]
On Mon, 4 Mar 2024 03:41:55 GMT, David Holmes wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> rename after merge: jvmti_common.h to jvmti_common.hpp
>
> test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java
> line 132:
>
>> 130: // count of threads waiting to re-enter:0
>> 131: // count of threads waiting to be notified:
>> NUMBER_OF_WAITING_THREADS
>> 132: check(lockCheck, null, 0, // no owner thread
>
> AFAICS you are racing here, as the waiting threads can set `ready` but not
> yet have called `wait()`. To know for sure they are in `wait()` you need to
> acquire the monitor before checking (and release it again so that you test
> the no-owner scenario as intended). But this should probably be done inside
> `startWaitingThreads`.
Thank you for the comment. Will implement similar approach as in the
`startEnteringThreads()`.
> test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java
> line 319:
>
>> 317: try {
>> 318: ready = true;
>> 319: lockCheck.wait();
>
> Spurious wakeups will break this test. They shouldn't happen but ...
Thanks, will check how this can be fixed.
> test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/libObjectMonitorUsage.cpp
> line 65:
>
>> 63:
>> 64:
>> 65: jint Agent_Initialize(JavaVM *jvm, char *options, void *reserved) {
>
> Nit: there's a double space after jint
Will fix, thanks,
> test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/libObjectMonitorUsage.cpp
> line 219:
>
>> 217: }
>> 218:
>> 219: } // exnern "C"
>
> typo: exnern -> extern
Will fix, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510912362
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510913702
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510909857
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510909512
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v21]
On Mon, 4 Mar 2024 03:20:41 GMT, David Holmes wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> rename after merge: jvmti_common.h to jvmti_common.hpp
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1505:
>
>> 1503: // the monitor threads after being notified. Here we are
>> correcting the actual
>> 1504: // number of the waiting threads by excluding those
>> re-entering the monitor.
>> 1505: nWait = i;
>
> Sorry I can't follow the logic here. Why is the whole block not simply:
>
> if (mon != nullptry) {
> // The nWait count may include threads trying to re-enter the monitor, so
> // walk the actual set of waiters to count those actually waiting.
> int count = 0;
> for (ObjectMonitor * waiter = mon->first_waiter(); waiter != nullptr;
> waiter = mon->next_waiter(waiter)) {
> count++;
> }
> }
> ret.notify_waiter_count = count;
Thank you for the suggestion. Let me check this.
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1552:
>
>> 1550: // If the thread was found on the ObjectWaiter list, then
>> 1551: // it has not been notified. This thread can't change the
>> 1552: // state of the monitor so it doesn't need to be suspended.
>
> Not sure why suspension is being mentioned here.
This is the original comment. I also do not quite understand it. It is
confusing for sure. I can remove it if Dan does not object.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510906350
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510904664
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v21]
On Mon, 4 Mar 2024 03:07:58 GMT, David Holmes wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> rename after merge: jvmti_common.h to jvmti_common.hpp > > src/hotspot/share/prims/jvmtiEnvBase.cpp line 1489: > >> 1487: assert(mon != nullptr, "must have monitor"); >> 1488: // this object has a heavyweight monitor >> 1489: nWant = mon->contentions(); // # of threads contending for monitor > > Please clarify comment as > > // # of threads contending for monitor entry, but not re-entry > > (Fixed typos - sorry) Will fix, thanks. > src/hotspot/share/prims/jvmtiEnvBase.cpp line 1490: > >> 1488: // this object has a heavyweight monitor >> 1489: nWant = mon->contentions(); // # of threads contending for monitor >> 1490: nWait = mon->waiters(); // # of threads in Object.wait() > > Please clarify the comment as > > // # of threads waiting for notification, or to re-enter monitor, in > Object.wait() Good catch, thanks. Will fix. > src/hotspot/share/prims/jvmtiEnvBase.cpp line 1491: > >> 1489: nWant = mon->contentions(); // # of threads contending for monitor >> 1490: nWait = mon->waiters(); // # of threads in Object.wait() >> 1491: wantList = Threads::get_pending_threads(tlh.list(), nWant + >> nWait, (address)mon); > > Please add a comment > > // Get the actual set of threads trying to enter, or re-enter, the monitor. Will add, thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510895824 PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510894066 PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1510896692
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v21]
On Fri, 1 Mar 2024 11:50:05 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> rename after merge: jvmti_common.h to jvmti_common.hpp
Thanks Serguei, this is generally looking good, but I have one main query with
the logic, a number of suggestions regarding comments, and a few issues in the
new test code.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1489:
> 1487: assert(mon != nullptr, "must have monitor");
> 1488: // this object has a heavyweight monitor
> 1489: nWant = mon->contentions(); // # of threads contending for monitor
Please clarity comment as
// # of threads contending for monitor entry, but not -rentry
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1490:
> 1488: // this object has a heavyweight monitor
> 1489: nWant = mon->contentions(); // # of threads contending for monitor
> 1490: nWait = mon->waiters(); // # of threads in Object.wait()
Please clarify the comment as
// # of threads waiting for notification, or to re-enter monitor, in
Object.wait()
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1491:
> 1489: nWant = mon->contentions(); // # of threads contending for monitor
> 1490: nWait = mon->waiters(); // # of threads in Object.wait()
> 1491: wantList = Threads::get_pending_threads(tlh.list(), nWant + nWait,
> (address)mon);
Please add a comment
// Get the actual set of threads trying to enter, or re-enter, the monitor.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1505:
> 1503: // the monitor threads after being notified. Here we are
> correcting the actual
> 1504: // number of the waiting threads by excluding those re-entering
> the monitor.
> 1505: nWait = i;
Sorry I can't follow the logic here. Why is the whole block not simply:
if (mon != nullptry) {
// The nWait count may include threads trying to re-enter the monitor, so
// walk the actual set of waiters to count those actually waiting.
int count = 0;
for (ObjectMonitor * waiter = mon->first_waiter(); waiter != nullptr; waiter
= mon->next_waiter(waiter)) {
count++;
}
}
ret.notify_waiter_count = count;
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1552:
> 1550: // If the thread was found on the ObjectWaiter list, then
> 1551: // it has not been notified. This thread can't change the
> 1552: // state of the monitor so it doesn't need
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v21]
On Fri, 1 Mar 2024 11:50:05 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> rename after merge: jvmti_common.h to jvmti_common.hpp
> The notify_waiters has to include all threads waiting to be notified in
> Object.wait().
> The implementation also includes the threads waiting to re-enter the monitor
> in Object.wait() which is wrong.
This is not in fact the case so can you please remove this from the problem
statement/description above.
-
PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1975561224
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v8]
On Wed, 14 Feb 2024 21:13:56 GMT, Daniel D. Daugherty wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: added assert to get_pending_threads; added suggested coverage to >> test objmonusage003 > > CHECK_FOR_BAD_RESULTS and ADD_DELAYS_FOR_RACES flags and > their associated code paths are for temporary debugging only. @dcubed-ojdk @dholmes-ora Dan and David, I think, I've addressed all your comments. Do you have any other concerns? Can you approve it now? Does the RN look okay? - PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1973668452
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v21]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
rename after merge: jvmti_common.h to jvmti_common.hpp
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/f1a97f51..b449f04a
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=20
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=19-20
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v20]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 22 commits:
- Merge
- review: update comment in threads.hpp
- fix deadlock with carrier threads starvation in ObjectMonitorUsage test
- resolve merge conflict for deleted file objmonusage003.cpp
- fix a typo in libObjectMonitorUsage.cpp
- fix potential sync gap in the test ObjectMonitorUsage
- improve ObjectMonitorUsage test native agent output
- improved the ObjectMonitorUsage test to make it more elegant
- review: remove test objmonusage003; improve test ObjectMonitorUsage
- review: addressed minor issue with use of []; corrected the test desctiption
- ... and 12 more: https://git.openjdk.org/jdk/compare/e85265ab...f1a97f51
-
Changes: https://git.openjdk.org/jdk/pull/17680/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=17680=19
Stats: 1123 lines in 15 files changed: 686 ins; 389 del; 48 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v19]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with two additional
commits since the last revision:
- review: update comment in threads.hpp
- fix deadlock with carrier threads starvation in ObjectMonitorUsage test
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/06711644..7244d349
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=18
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=17-18
Stats: 4 lines in 2 files changed: 2 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v18]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 19 commits:
- resolve merge conflict for deleted file objmonusage003.cpp
- fix a typo in libObjectMonitorUsage.cpp
- fix potential sync gap in the test ObjectMonitorUsage
- improve ObjectMonitorUsage test native agent output
- improved the ObjectMonitorUsage test to make it more elegant
- review: remove test objmonusage003; improve test ObjectMonitorUsage
- review: addressed minor issue with use of []; corrected the test desctiption
- review: addressed comments from David
- fixed minimal build issue
- cloned an nsk/jvmti test to provide test coverage for virtual threads; fixed
vthread related issue
- ... and 9 more: https://git.openjdk.org/jdk/compare/5fa2bdc6...06711644
-
Changes: https://git.openjdk.org/jdk/pull/17680/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=17680=17
Stats: 1120 lines in 14 files changed: 684 ins; 389 del; 47 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v17]
On Tue, 27 Feb 2024 18:24:04 GMT, Leonid Mesnik wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> fix a typo in libObjectMonitorUsage.cpp > > src/hotspot/share/runtime/threads.cpp line 1185: > >> 1183: } >> 1184: >> 1185: #if INCLUDE_JVMTI > > Is it needed to update thread.hpp also? Thank you for the comment, will check it. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1504802182
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v17]
On Fri, 23 Feb 2024 18:40:10 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fix a typo in libObjectMonitorUsage.cpp
Thank you for review, Leonid!
-
PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1967416721
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v17]
On Fri, 23 Feb 2024 18:40:10 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> fix a typo in libObjectMonitorUsage.cpp
There is one comment about changing #define for thread.hpp.
The test changes look good for me. Thanks for converting the test from
vmTetbase.
src/hotspot/share/runtime/threads.cpp line 1185:
> 1183: }
> 1184:
> 1185: #if INCLUDE_JVMTI
Is it needed to update thread.hpp also?
-
Marked as reviewed by lmesnik (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17680#pullrequestreview-1904467409
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1504740367
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v17]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
fix a typo in libObjectMonitorUsage.cpp
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/fd507055..091fd29c
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=16
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=15-16
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v16]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with two additional
commits since the last revision:
- fix potential sync gap in the test ObjectMonitorUsage
- improve ObjectMonitorUsage test native agent output
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/716deae2..fd507055
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=15
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=14-15
Stats: 146 lines in 2 files changed: 114 ins; 20 del; 12 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v15]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
improved the ObjectMonitorUsage test to make it more elegant
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/ef779169..716deae2
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=14
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=13-14
Stats: 105 lines in 1 file changed: 39 ins; 44 del; 22 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v14]
On Thu, 22 Feb 2024 11:22:26 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: remove test objmonusage003; improve test ObjectMonitorUsage
I've pushed an update which addresses test related review comments from Leonid
and adds some test improvements:
- remove the `nsk/jvmti` test `objmonusage003` which was converted into
ObjectMonitorUsage
- rename the test folder from `GetObjectMonitorUsage` to `ObjectMonitorUsage`
for naming unification
- add one more testing scenario: `test0`
- add test specific thread names
- address several minor review comments from Leonid
- add better organized log messages
-
PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1959255417
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v14]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
review: remove test objmonusage003; improve test ObjectMonitorUsage
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/e095cffe..ef779169
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=13
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=12-13
Stats: 1353 lines in 9 files changed: 454 ins; 899 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v13]
On Wed, 21 Feb 2024 22:33:51 GMT, Leonid Mesnik wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: addressed minor issue with use of []; corrected the test >> desctiption > > test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage003.java > line 31: > >> 29: >> 30: final static int JCK_STATUS_BASE = 95; >> 31: final static int NUMBER_OF_THREADS = 16; > > Better to remove the test if it already converted to avoid mess. Agreed. I was thinking about the same. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498773675
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v13]
On Wed, 21 Feb 2024 22:34:19 GMT, Leonid Mesnik wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: addressed minor issue with use of []; corrected the test
>> desctiption
>
> test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/ObjectMonitorUsage.java
> line 42:
>
>> 40: * - all the above is checked for both platform and virtual threads
>> 41: * @requires vm.jvmti
>> 42: * @compile ObjectMonitorUsage.java
>
> No need to have explicit compile for the test code.
Fixed, thanks.
> test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/ObjectMonitorUsage.java
> line 204:
>
>> 202:
>> 203: // test virtual threads
>> 204: test1(false);
>
> shouldn't be true here?
Nice catch, thanks. Fixed now.
> test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/libObjectMonitorUsage.cpp
> line 105:
>
>> 103: err = jvmti->GetObjectMonitorUsage(obj, );
>> 104: if (err == JVMTI_ERROR_MUST_POSSESS_CAPABILITY &&
>> !caps.can_get_monitor_info) {
>> 105: return; /* Ok, it's expected */
>
> I think we don't need this path.
Yes. Fixed, thanks.
> test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/libObjectMonitorUsage.cpp
> line 107:
>
>> 105: return; /* Ok, it's expected */
>> 106: } else if (err != JVMTI_ERROR_NONE) {
>> 107: LOG("(GetMonitorInfo#%d) unexpected error: %s (%d)\n",
>
> you could use check_jvmti_status
Fixed, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498694748
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498694426
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498693585
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498693206
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v13]
On Wed, 21 Feb 2024 22:36:31 GMT, Leonid Mesnik wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: addressed minor issue with use of []; corrected the test
>> desctiption
>
> test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/ObjectMonitorUsage.java
> line 72:
>
>> 70: * - zero threads waiting to be notified
>> 71: */
>> 72: static void test1(boolean isVirtual) throws Error {
>
> no need to add throws for unchecked excption
Fixed, thanks.
> test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/libObjectMonitorUsage.cpp
> line 117:
>
>> 115: LOG(">>> [%2d]owner: none (0x0)\n", count);
>> 116: } else {
>> 117: err = jvmti->GetThreadInfo(inf.owner, );
>
> need to check err status.
Fixed, thanks.
> test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/libObjectMonitorUsage.cpp
> line 126:
>
>> 124: LOG(">>> waiters:\n");
>> 125: for (j = 0; j < inf.waiter_count; j++) {
>> 126: err = jvmti->GetThreadInfo(inf.waiters[j], );
>
> need to check err.
Done, thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498688861
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498690913
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498688037
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v13]
On Wed, 21 Feb 2024 22:23:27 GMT, Leonid Mesnik wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: addressed minor issue with use of []; corrected the test
>> desctiption
>
> test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/ObjectMonitorUsage.java
> line 48:
>
>> 46: public class ObjectMonitorUsage {
>> 47:
>> 48: final static int JCK_STATUS_BASE = 95;
>
> JCK_STATUS_BASE is not used, need to be removed.
Thanks, done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1498672751
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v13]
On Tue, 20 Feb 2024 23:48:06 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: addressed minor issue with use of []; corrected the test desctiption
Leonid, thank you for the comments. Will work on addressing them.
-
PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1958221870
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v13]
On Tue, 20 Feb 2024 23:48:06 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: addressed minor issue with use of []; corrected the test desctiption
Changes requested by lmesnik (Reviewer).
test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/ObjectMonitorUsage.java
line 42:
> 40: * - all the above is checked for both platform and virtual threads
> 41: * @requires vm.jvmti
> 42: * @compile ObjectMonitorUsage.java
No need to have explicit compile for the test code.
test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/ObjectMonitorUsage.java
line 48:
> 46: public class ObjectMonitorUsage {
> 47:
> 48: final static int JCK_STATUS_BASE = 95;
JCK_STATUS_BASE is not used, need to be removed.
test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/ObjectMonitorUsage.java
line 72:
> 70: * - zero threads waiting to be notified
> 71: */
> 72: static void test1(boolean isVirtual) throws Error {
no need to add throws for unchecked excption
test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/ObjectMonitorUsage.java
line 204:
> 202:
> 203: // test virtual threads
> 204: test1(false);
shouldn't be true here?
test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/libObjectMonitorUsage.cpp
line 37:
> 35: static jvmtiCapabilities caps;
> 36: static jint result = PASSED;
> 37: static jboolean printdump = JNI_FALSE;
if there are not too much logging, better just to enable log by default and
remove this variable
test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/libObjectMonitorUsage.cpp
line 105:
> 103: err = jvmti->GetObjectMonitorUsage(obj, );
> 104: if (err == JVMTI_ERROR_MUST_POSSESS_CAPABILITY &&
> !caps.can_get_monitor_info) {
> 105: return; /* Ok, it's expected */
I think we don't need this path.
test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/libObjectMonitorUsage.cpp
line 107:
> 105: return; /* Ok, it's expected */
> 106: } else if (err != JVMTI_ERROR_NONE) {
> 107: LOG("(GetMonitorInfo#%d) unexpected error: %s (%d)\n",
you could use check_jvmti_status
test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/libObjectMonitorUsage.cpp
line 117:
> 115: LOG(">>> [%2d]owner: none (0x0)\n", count);
> 116: } else {
> 117: err = jvmti->GetThreadInfo(inf.owner,
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v12]
On Tue, 20 Feb 2024 06:15:07 GMT, David Holmes wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: addressed comments from David > > test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/ObjectMonitorUsage.java > line 54: > >> 52: >> 53: static Object lockCheck = new Object(); >> 54: static Thread thr[] = new Thread[NUMBER_OF_THREADS]; > > Nit: the `[]` go on the type not the variable. Thanks, fixed now. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1496688629
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v13]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
review: addressed minor issue with use of []; corrected the test desctiption
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/81b7787b..e095cffe
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=12
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=11-12
Stats: 13 lines in 1 file changed: 5 ins; 5 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v12]
On Mon, 19 Feb 2024 15:33:23 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: addressed comments from David
test/hotspot/jtreg/serviceability/jvmti/GetObjectMonitorUsage/ObjectMonitorUsage.java
line 54:
> 52:
> 53: static Object lockCheck = new Object();
> 54: static Thread thr[] = new Thread[NUMBER_OF_THREADS];
Nit: the `[]` go on the type not the variable.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1495292810
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v12]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
review: addressed comments from David
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/63706e51..81b7787b
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=11
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=10-11
Stats: 12 lines in 1 file changed: 8 ins; 4 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v9]
On Fri, 16 Feb 2024 06:09:40 GMT, David Holmes wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: JDWP monitor_info spec clarification; removed debugging code from
>> objmonusage001
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1500:
>
>> 1498: for (int i = 0; i < nWait; i++) {
>> 1499: if (waiter == nullptr || (i != 0 && waiter ==
>> mon->first_waiter())) {
>> 1500: // robustness: the waiting list has gotten smaller
>
> We are at a safepoint so I don't see how the wait list can shrink. I
> initially thought perhaps a waiter could timeout, but the code that does the
> timed park is wrapped in ` ThreadBlockInVMPreprocess` which will block at a
> safepoint if one is active.
Thank you for the question.
The `nWait` count we got from the `mon->waiters()` can include the threads that
are re-entering the monitor after being notified. Here we are correcting the
actual number of the waiting threads by excluding those re-entering the
monitor. The comment need an update to explain it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1494556207
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v9]
On Fri, 16 Feb 2024 06:01:19 GMT, David Holmes wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: JDWP monitor_info spec clarification; removed debugging code from
>> objmonusage001
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1496:
>
>> 1494: nWant = wantList->length();
>> 1495:
>> 1496: if (mon != nullptr) {
>
> Shouldn't the call to `get_pending_threads` only happen if `mon != nullptr`?
> Otherwise the `wantList` has to be empty.
Good catch. I've moved it under the `if (mon != nullptr) {` condition.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1494547012
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v11]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
fixed minimal build issue
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/04da0cf8..63706e51
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=10
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=09-10
Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v10]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
cloned an nsk/jvmti test to provide test coverage for virtual threads; fixed
vthread related issue
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/c8b8e376..04da0cf8
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=09
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=08-09
Stats: 431 lines in 5 files changed: 415 ins; 11 del; 5 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v9]
On Thu, 15 Feb 2024 19:55:08 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: JDWP monitor_info spec clarification; removed debugging code from
> objmonusage001
Thanks for the test updates.
A couple of other queries.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1496:
> 1494: nWant = wantList->length();
> 1495:
> 1496: if (mon != nullptr) {
Shouldn't the call to `get_pending_threads` only happen if `mon != nullptr`?
Otherwise the `wantList` has to be empty.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1500:
> 1498: for (int i = 0; i < nWait; i++) {
> 1499: if (waiter == nullptr || (i != 0 && waiter ==
> mon->first_waiter())) {
> 1500: // robustness: the waiting list has gotten smaller
We are at a safepoint so I don't see how the wait list can shrink. I initially
thought perhaps a waiter could timeout, but the code that does the timed park
is wrapped in ` ThreadBlockInVMPreprocess` which will block at a safepoint if
one is active.
-
PR Review: https://git.openjdk.org/jdk/pull/17680#pullrequestreview-1884365884
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1491992136
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1491997307
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v9]
On Thu, 15 Feb 2024 19:55:08 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: JDWP monitor_info spec clarification; removed debugging code from
> objmonusage001
I've pushed an update that includes:
- added minor spec clarification for JDWP `ObjectReference.MonitorInfo`
command (please, see the CSR)
- removed the CHECK_FOR_BAD_RESULTS debugging mode from the test:
`nsk/jvmti/GetObjectMonitorUsage/objmonusage001`
-
PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1947208935
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v9]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
review: JDWP monitor_info spec clarification; removed debugging code from
objmonusage001
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/1ab5b349..c8b8e376
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=08
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=07-08
Stats: 25 lines in 2 files changed: 0 ins; 13 del; 12 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v4]
On Wed, 14 Feb 2024 01:52:16 GMT, David Holmes wrote: >> Thank you for the comment, David. >> Now the test checks: >> - the threads waiting to enter the monitor are returned correctly with all >> permutations of threads waiting to be notified and threads waiting to >> re-enter the monitor >> Initially, there are 4 threads waiting to be notified and zero threads >> waiting to re-enter the monitor. >> They are notified one by with the subsequent checks, so all the pairs are >> checked ``: `<0,4>, <1,3>, <2,2>, <3,1>, <4,0>` >> >> I'm not sure why do you think all permutations covering zero and non-zero >> for each one are needed. >> At least, it looks like an overkill to me. But if you still think it is >> really important then I can add cases with zero threads waiting to enter the >> monitor with the same permutations of threads waiting to be notified and >> threads re-entering the monitor. > > If you don't check all the zero/non-zero permutations for the three counts of > interest then you don't know that you are combining them the right way to > give the two counts reported by the API. > Note that checking "all the pairs" is not really necessary as all non-zero > values fall in the same equivalence class for testing purposes. The last suggestion has been addressed by adding missing test cases to the `nsk/jvmti/GetObjectMonitorUsage/objmonusage003` test. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1490889210
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v5]
On Wed, 14 Feb 2024 12:16:38 GMT, Serguei Spitsyn wrote: >> src/hotspot/share/runtime/threads.cpp line 1200: >> >>> 1198: if (pending == monitor || >>> 1199: (waiting == monitor && >>> JavaThreadStatus::BLOCKED_ON_MONITOR_ENTER == >>> 1200: java_lang_Thread::get_thread_status(p->vthread_or_thread())) >> >> This code seems extremely racy. Is there anything that is stopping the >> target thread from running while we attempt this inspection of the oop >> state? If it was waiting and was interrupted or timed-out then we could >> mistakenly think it was still pending entry to this monitor when in fact it >> is not. >> It is also a shame that we have to reach up into the oop thread state to >> figure out if if it is blocked on re-entry ... > > The function `get_pending_threads()` is executed only by the > VM_GetObjectMonitorUsage operation. > I've added this assert there: > `assert(Thread::current()->is_VM_thread(), "Must be the VM thread");` > Does it address your concern? Yes thanks. Didn't realize we were in a safepoint VM op. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1490510763
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v8]
On Wed, 14 Feb 2024 15:25:33 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: added assert to get_pending_threads; added suggested coverage to
> test objmonusage003
CHECK_FOR_BAD_RESULTS and ADD_DELAYS_FOR_RACES flags and
their associated code paths are for temporary debugging only.
-
PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1944611040
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v4]
On Wed, 14 Feb 2024 18:33:19 GMT, Daniel D. Daugherty wrote: >>> When you get the chance, can you checkout these possible >> changes for the objmonusage001 test? >> >> Thanks, Dan. I've pushed the suggested test changes but refactored them a >> little bit. > >> Thanks, Dan. I've pushed the suggested test changes but refactored them a >> little bit. > > You are welcome. I presume the revised test passes with your fix in place. I > made the > test changes on a baseline repo and not a repo that had your changes which is > why I > had temporary debug flags in the test.. > > Also, do you agree with my test assertion comments in the verification point > comments? > Am I properly understanding how we are changing this API? >From @dcubed-ojdk : > Also, do you agree with my test assertion comments in the verification point > comments? > Am I properly understanding how we are changing this API? Yes, I agree. All looks right to me. Q: Do you want to keep the `CHECK_FOR_BAD_RESULTS` mode or it can be removed after the failing output was verified? - PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1944545272
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v4]
On Wed, 14 Feb 2024 11:56:27 GMT, Serguei Spitsyn wrote: > Thanks, Dan. I've pushed the suggested test changes but refactored them a > little bit. You are welcome. I presume the revised test passes with your fix in place. I made the test changes on a baseline repo and not a repo that had your changes which is why I had temporary debug flags in the test.. Also, do you agree with my test assertion comments in the verification point comments? Am I properly understanding how we are changing this API? - PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1944382731
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v3]
On Thu, 8 Feb 2024 07:05:38 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 three additional >> commits since the last revision: >> >> - Merge >> - review: thread in notify waiter list can't be BLOCKED >> - 8324677: Specification clarification needed for JVM TI >> GetObjectMonitorUsage > > Based on Dan's analysis I would have to go back and redo the complete > analysis for myself. :( I think the only way to make sense of this is to > actually set up scenarios where we have different threads contending on > entry, different threads waiting and different threads re-entering after > being notified, and see what values actually get returned in each case. > > I think the `pending_current_monitor` issue may be a distinct/different > issue. I can easily imagine that this was overlooked when we introduced the > "wait morphing" rather than have the notified/timed-out/interrupted waiters > actually place themselves on the entry queue. >From @dholmes-ora : > If you don't check all the zero/non-zero permutations for the three counts of > interest then you don't know that you are combining them the right way to > give the two counts reported by the API. Okay, thanks. I've added the suggested test cases to the `nsk/jvmti/GetObjectMonitorUsage/objmonusage003`. One of the testcases is covered by the `nsk/jvmti/GetObjectMonitorUsage/objmonusage001`. - PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1944052748
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v8]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
review: added assert to get_pending_threads; added suggested coverage to test
objmonusage003
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/674da98c..1ab5b349
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=07
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=06-07
Stats: 132 lines in 2 files changed: 106 ins; 0 del; 26 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v5]
On Wed, 14 Feb 2024 02:18:58 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 five additional >> commits since the last revision: >> >> - Merge >> - review: fixed issues in get_object_monitor_usage; extended test coverage >> in objmonusage003 >> - Merge >> - review: thread in notify waiter list can't be BLOCKED >> - 8324677: Specification clarification needed for JVM TI >> GetObjectMonitorUsage > > src/hotspot/share/runtime/threads.cpp line 1200: > >> 1198: if (pending == monitor || >> 1199: (waiting == monitor && >> JavaThreadStatus::BLOCKED_ON_MONITOR_ENTER == >> 1200: java_lang_Thread::get_thread_status(p->vthread_or_thread())) > > This code seems extremely racy. Is there anything that is stopping the target > thread from running while we attempt this inspection of the oop state? If it > was waiting and was interrupted or timed-out then we could mistakenly think > it was still pending entry to this monitor when in fact it is not. > It is also a shame that we have to reach up into the oop thread state to > figure out if if it is blocked on re-entry ... The function `get_pending_threads()` is executed only by the VM_GetObjectMonitorUsage operation. I've added this assert there: `assert(Thread::current()->is_VM_thread(), "Must be the VM thread");` Does it address your concern? - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1489379382
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v7]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
fixed trailing spaces in one line
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/070b15d1..674da98c
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=06
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=05-06
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v4]
On Tue, 13 Feb 2024 22:30:56 GMT, Daniel D. Daugherty wrote: > When you get the chance, can you checkout these possible changes for the objmonusage001 test? Thanks, Dan. I've pushed the suggested test changes but refactored them a little bit. - PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1943626874
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v6]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
review: imported the suggestion extending the nsk/jvmti test objmonusage001
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/182cd07c..070b15d1
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=05
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=04-05
Stats: 133 lines in 3 files changed: 118 ins; 0 del; 15 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v5]
On Tue, 13 Feb 2024 07:11:18 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with 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 five additional
> commits since the last revision:
>
> - Merge
> - review: fixed issues in get_object_monitor_usage; extended test coverage
> in objmonusage003
> - Merge
> - review: thread in notify waiter list can't be BLOCKED
> - 8324677: Specification clarification needed for JVM TI
> GetObjectMonitorUsage
> Why do you think the virtual threads are handled incorrectly?
Sorry my mistake. The `thread_or_vthread` changes had me confused.
src/hotspot/share/runtime/threads.cpp line 1200:
> 1198: if (pending == monitor ||
> 1199: (waiting == monitor &&
> JavaThreadStatus::BLOCKED_ON_MONITOR_ENTER ==
> 1200: java_lang_Thread::get_thread_status(p->vthread_or_thread()))
This code seems extremely racy. Is there anything that is stopping the target
thread from running while we attempt this inspection of the oop state? If it
was waiting and was interrupted or timed-out then we could mistakenly think it
was still pending entry to this monitor when in fact it is not.
It is also a shame that we have to reach up into the oop thread state to figure
out if if it is blocked on re-entry ...
-
PR Review: https://git.openjdk.org/jdk/pull/17680#pullrequestreview-1879239973
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1488808389
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v4]
On Tue, 13 Feb 2024 08:34:57 GMT, Serguei Spitsyn wrote: >> test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage003.java >> line 33: >> >>> 31: final static int NUMBER_OF_ENTERER_THREADS = 4; >>> 32: final static int NUMBER_OF_WAITER_THREADS = 4; >>> 33: final static int NUMBER_OF_THREADS = NUMBER_OF_ENTERER_THREADS + >>> NUMBER_OF_WAITER_THREADS; >> >> This testing is not sufficient. We have three states of interest: >> - entering the monitor directly >> - waiting in the monitor via Object.wait >> - re-rentering the monitor after being notified (or timed-out or interrupted) >> - we need to check all of these conditions in permutations covering zero and >> non-zero for each one, and then see that we get the correct counts back from >> JVM TI. > > Thank you for the comment, David. > Now the test checks: > - the threads waiting to enter the monitor are returned correctly with all > permutations of threads waiting to be notified and threads waiting to > re-enter the monitor > Initially, there are 4 threads waiting to be notified and zero threads > waiting to re-enter the monitor. > They are notified one by with the subsequent checks, so all the pairs are > checked ``: `<0,4>, <1,3>, <2,2>, <3,1>, <4,0>` > > I'm not sure why do you think all permutations covering zero and non-zero for > each one are needed. > At least, it looks like an overkill to me. But if you still think it is > really important then I can add cases with zero threads waiting to enter the > monitor with the same permutations of threads waiting to be notified and > threads re-entering the monitor. If you don't check all the zero/non-zero permutations for the three counts of interest then you don't know that you are combining them the right way to give the two counts reported by the API. Note that checking "all the pairs" is not really necessary as all non-zero values fall in the same equivalence class for testing purposes. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1488795667
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v4]
On Tue, 13 Feb 2024 08:39:38 GMT, Serguei Spitsyn wrote: >> Sorry really struggling to understand this now. We have gone from a simple >> miscalculation to apparently doing everything wrong. >> >> IIUC this API does not currently handle virtual threads correctly -i s that >> the case? If so I would like to see that fix factored out and done >> separately before this fix is done. Thanks. > >> Sorry really struggling to understand this now. We have gone from a simple >> miscalculation to apparently doing everything wrong. IIUC this API does not >> currently handle virtual threads correctly - is that the case? If so I would >> like to see that fix factored out and done separately before this fix is >> done. Thanks. > > Thank you for the concern, David. But it is not clear what it is. Could you, > please, explain a little bit? > Why do you think the virtual threads are handled incorrectly? @sspitsyn - When you get the chance, can you checkout these possible changes for the objmonusage001 test? https://github.com/openjdk/jdk/pull/17839 Summary of my test changes: - extend the check() function to verify more of the jvmtiMonitorUsage fields - add detailed comments at each of the verification points so it's clear what is being verified and why - add a better summary for the overall test - make it clear which output lines document failures I've also made some temporary debugging changes that are only useful while updating this test: - temporarily use the "printdump" parameter to enable test output - add a flag to verify the current JVMs bad jvmtiMonitorUsage fields so that the test can pass with an unmodified JVM - add a flag to add a delay that helps check for a lack of races in the verification points These proposed test changes help document how I think the GetObjectMonitorUsage API is supposed to work on a simple test case. - PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1942747157
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v4]
On Tue, 13 Feb 2024 07:08:33 GMT, David Holmes wrote: > Sorry really struggling to understand this now. We have gone from a simple > miscalculation to apparently doing everything wrong. IIUC this API does not > currently handle virtual threads correctly - is that the case? If so I would > like to see that fix factored out and done separately before this fix is > done. Thanks. Thank you for the concern, David. But it is not clear what it is. Could you, please, explain a little bit? Why do you think the virtual threads are handled incorrectly? - PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1940760918
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v4]
On Tue, 13 Feb 2024 07:06:05 GMT, David Holmes wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: fixed issues in get_object_monitor_usage; extended test coverage >> in objmonusage003 > > test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage003.java > line 33: > >> 31: final static int NUMBER_OF_ENTERER_THREADS = 4; >> 32: final static int NUMBER_OF_WAITER_THREADS = 4; >> 33: final static int NUMBER_OF_THREADS = NUMBER_OF_ENTERER_THREADS + >> NUMBER_OF_WAITER_THREADS; > > This testing is not sufficient. We have three states of interest: > - entering the monitor directly > - waiting in the monitor via Object.wait > - re-rentering the monitor after being notified (or timed-out or interrupted) > - we need to check all of these conditions in permutations covering zero and > non-zero for each one, and then see that we get the correct counts back from > JVM TI. Thank you for the comment, David. Now the test checks: - the threads waiting to enter the monitor are returned correctly with all permutations of threads waiting to be notified and threads waiting to re-enter the monitor Initially, there are 4 threads waiting to be notified and zero threads waiting to re-enter the monitor. They are notified one by with the subsequent checks, so all the pairs are checked ``: `<0,4>, <1,3>, <2,2>, <3,1>, <4,0>` I'm not sure why do you think all permutations covering zero and non-zero for each one are needed. At least, it looks like an overkill to me. But if you still think it is really important then I can add cases with zero threads waiting to enter the monitor with the same permutations of threads waiting to be notified and threads re-entering the monitor. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1487402314
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v4]
On Tue, 13 Feb 2024 06:55:46 GMT, David Holmes wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: fixed issues in get_object_monitor_usage; extended test coverage
>> in objmonusage003
>
> src/hotspot/share/runtime/javaThread.cpp line 196:
>
>> 194: oop JavaThread::vthread_or_thread() const {
>> 195: oop result = vthread();
>> 196: if (result == nullptr) {
>
> Is that really sufficient? If so why do we have `is_vthread_mounted` which
> checks the continuation?
Thank you for the question.
This function does not care if this is a vthread, or not and if it is mounted
or not.
We just need a right oop of currently executed thread or vthread.
The `threadOop()` and `vthread()` can return the same oop.
If `vthread()` is not `nullptr` then it can point either to a `j.l.Thread` or a
`j.lVirtualThread`.
If it is `nullptr` then we have to take `threadOop()`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1487374609
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v4]
On Tue, 13 Feb 2024 07:00:41 GMT, David Holmes wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: fixed issues in get_object_monitor_usage; extended test coverage >> in objmonusage003 > > test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage003.java > line 31: > >> 29: >> 30: final static int JCK_STATUS_BASE = 95; >> 31: final static int NUMBER_OF_ENTERER_THREADS = 4; > > Nit: ENTERING not ENTERER Okay, thanks. Will rename it. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1487366808
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v4]
On Sat, 10 Feb 2024 04:06:37 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: fixed issues in get_object_monitor_usage; extended test coverage in
> objmonusage003
Sorry really struggling to understand this now. We have gone from a simple
miscalculation to apparently doing everything wrong.
IIUC this API does not currently handle virtual threads correctly -i s that the
case? If so I would like to see that fix factored out and done separately
before this fix is done. Thanks.
src/hotspot/share/runtime/javaThread.cpp line 196:
> 194: oop JavaThread::vthread_or_thread() const {
> 195: oop result = vthread();
> 196: if (result == nullptr) {
Is that really sufficient? If so why do we have `is_vthread_mounted` which
checks the continuation?
test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage003.java
line 31:
> 29:
> 30: final static int JCK_STATUS_BASE = 95;
> 31: final static int NUMBER_OF_ENTERER_THREADS = 4;
Nit: ENTERING not ENTERER
test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage003.java
line 33:
> 31: final static int NUMBER_OF_ENTERER_THREADS = 4;
> 32: final static int NUMBER_OF_WAITER_THREADS = 4;
> 33: final static int NUMBER_OF_THREADS = NUMBER_OF_ENTERER_THREADS +
> NUMBER_OF_WAITER_THREADS;
This testing is not sufficient. We have three states of interest:
- entering the monitor directly
- waiting in the monitor via Object.wait
- re-rentering the monitor after being notified (or timed-out or interrupted)
- we need to check all of these conditions in permutations covering zero and
non-zero for each one, and then see that we get the correct counts back from
JVM TI.
-
PR Review: https://git.openjdk.org/jdk/pull/17680#pullrequestreview-1876986745
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1487299437
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1487304912
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1487311675
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v5]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with 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 five additional
commits since the last revision:
- Merge
- review: fixed issues in get_object_monitor_usage; extended test coverage in
objmonusage003
- Merge
- review: thread in notify waiter list can't be BLOCKED
- 8324677: Specification clarification needed for JVM TI GetObjectMonitorUsage
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/69ba5e7e..182cd07c
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=04
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=03-04
Stats: 6592 lines in 396 files changed: 3981 ins; 1259 del; 1352 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v4]
On Sat, 10 Feb 2024 04:06:37 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: fixed issues in get_object_monitor_usage; extended test coverage in
> objmonusage003
I've pushed an update addressing review comments from Dan/David.
This is a short list describing the updates:
- now the function `Threads::get_pending_threads()` in the `threads.cpp` is
supporting the monitor re-enter case
- introduced the function `oop JavaThread::vthread_or_thread()` used in the
function `Threads::get_pending_threads()`
- the fix in `jvmtiEnvBase.cpp` has been re-arranged to calculate early the
right sizes of the `waiters` and `notify_waiters` array
- the NSK test
`test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage003`
was extended to provide coverage suggested by David. It recreates a scenario
with several threads blocked on the monitor enter, several threads blocked on
re-enter and several thread waiting in the `Object.wait()` to be notified.
- some small test fixes suggested in review
The mach5 tiers 1-6 are all passed except the four known to fail JCK tests in
the tier 6.
-
PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1936927038
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v3]
On Thu, 8 Feb 2024 07:05:38 GMT, David Holmes wrote: > I think the only way to make sense of this is to actually set up scenarios > where we have different threads contending on entry, different threads > waiting and different threads re-entering after being notified, and see what > values actually get returned in each case. I've added this kind of test coverage to the test NSK test: `test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage003` > I think the `pending_current_monitor` issue may be a distinct/different > issue. I tried to find a solution fix the `pending_current_monitor` to cover the monitor re-enter case and found that it is not clear how to do it in a straight-forward way. So, decide to leave it alone for now. However, it seems, it could be more elegant to fix this function. I still can make it a try to do that if you give me some hints or ideas on how to do it better. - PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1936920693
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v4]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
review: fixed issues in get_object_monitor_usage; extended test coverage in
objmonusage003
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/3d735722..69ba5e7e
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=03
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=02-03
Stats: 155 lines in 8 files changed: 64 ins; 36 del; 55 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v3]
On Thu, 8 Feb 2024 15:29:47 GMT, Daniel D. Daugherty wrote: >> Yes, I've figured this out now. Thank you for pointing to it. >> It feels, the counts can be calculated correctly without touching the >> implementation of `current_pending_monitor()`, `current_waiting_monitor()`, >> `_contensions` and `_waiters`. >> At least, I'll try to fix it locally in the >> `JvmtiEnvBase::get_object_monitor_usage()`. >> Please, let me know what do you prefer. > > I don't have a preference (yet). Like what David suggested, I think we need to > determine the list of thread and monitor interaction scenarios that want to > exercise with this API. Once we're happy that those scenarios represent the > complete list of possible combinations we should double check that against the > API spec to make sure that those scenarios cover the API spec. Finally, we > need > to make sure that we have a test that covers all those scenarios. > > It has been a long, long time since I've looked at those NSK tests... :-( Thank you, Dan! I have a fix and will push it after the mach5 tiers testing. I've extended the test `test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage003` to have all combinations that David suggested but it was a little bit tricky to make it stable. It feels that an update of the current_pending_monitor() to report the re-entered monitors would make it more elegant. But I'm not sure how to do it correctly yet. Also, I've noticed we do not post the JVMTI events `MonitorContendedEnter` and `MonitorContendedEntered` for the re-enter case. It looks like a bug to me. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1484843163
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v3]
On Thu, 8 Feb 2024 10:34:14 GMT, Serguei Spitsyn wrote:
>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1521:
>>
>>> 1519: // we have contending and/or waiting threads
>>> 1520: if (nWant > 0) {
>>> 1521: // we have contending threads
>>
>> This block includes this logic:
>>
>> // get_pending_threads returns only java thread so we do not need to
>> // check for non java threads.
>> GrowableArray* wantList =
>> Threads::get_pending_threads(tlh.list(), nWant, (address)mon);
>> if (wantList->length() < nWant) {
>> // robustness: the pending list has gotten smaller
>> nWant = wantList->length();
>> }
>>
>> `Threads::get_pending_threads()` only returns threads where the
>> `current_pending_monitor` field is set for the specific monitor. That
>> happens only on contended enter and does not happen on contended
>> re-enter so this logic will already strip out any threads in `wait()` that
>> have not been notified and have not had their wait timers expire.
>> It will also strip out any waiters that have been notified or had
>> their wait timeouts expire.
>>
>> This means even if we fix the reenter code to increment the contentions
>> field, this logic will reduce that `nWant` value. Of course, the way around
>> that is to also update the reenter code to properly set the
>> `current_pending_monitor`
>> field and then the reentering threads won't be filtered out...
>
> Yes, I've figured this out now. Thank you for pointing to it.
> It feels, the counts can be calculated correctly without touching the
> implementation of `current_pending_monitor()`, `current_waiting_monitor()`,
> `_contensions` and `_waiters`.
> At least, I'll try to fix it locally in the
> `JvmtiEnvBase::get_object_monitor_usage()`.
> Please, let me know what do you prefer.
I don't have a preference (yet). Like what David suggested, I think we need to
determine the list of thread and monitor interaction scenarios that want to
exercise with this API. Once we're happy that those scenarios represent the
complete list of possible combinations we should double check that against the
API spec to make sure that those scenarios cover the API spec. Finally, we need
to make sure that we have a test that covers all those scenarios.
It has been a long, long time since I've looked at those NSK tests... :-(
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1483168890
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v3]
On Wed, 7 Feb 2024 22:37:55 GMT, Daniel D. Daugherty 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 three additional
>> commits since the last revision:
>>
>> - Merge
>> - review: thread in notify waiter list can't be BLOCKED
>> - 8324677: Specification clarification needed for JVM TI
>> GetObjectMonitorUsage
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1521:
>
>> 1519: // we have contending and/or waiting threads
>> 1520: if (nWant > 0) {
>> 1521: // we have contending threads
>
> This block includes this logic:
>
> // get_pending_threads returns only java thread so we do not need to
> // check for non java threads.
> GrowableArray* wantList =
> Threads::get_pending_threads(tlh.list(), nWant, (address)mon);
> if (wantList->length() < nWant) {
> // robustness: the pending list has gotten smaller
> nWant = wantList->length();
> }
>
> `Threads::get_pending_threads()` only returns threads where the
> `current_pending_monitor` field is set for the specific monitor. That
> happens only on contended enter and does not happen on contended
> re-enter so this logic will already strip out any threads in `wait()` that
> have not been notified and have not had their wait timers expire.
> It will also strip out any waiters that have been notified or had
> their wait timeouts expire.
>
> This means even if we fix the reenter code to increment the contentions
> field, this logic will reduce that `nWant` value. Of course, the way around
> that is to also update the reenter code to properly set the
> `current_pending_monitor`
> field and then the reentering threads won't be filtered out...
Yes, I've figured this out now. Thank you for pointing to it.
It feels, the counts can be calculated correctly without touching the
implementation of `current_pending_monitor()`, `current_waiting_monitor()`,
`_contensions` and `_waiters`.
At least, I'll try to fix it locally in the
`JvmtiEnvBase::get_object_monitor_usage()`.
Please, let me know what do you prefer.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1482749279
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v3]
On Wed, 7 Feb 2024 22:49:31 GMT, Daniel D. Daugherty 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 three additional >> commits since the last revision: >> >> - Merge >> - review: thread in notify waiter list can't be BLOCKED >> - 8324677: Specification clarification needed for JVM TI >> GetObjectMonitorUsage > > test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage001/objmonusage001.cpp > line 95: > >> 93: >> 94: JNIEXPORT void JNICALL >> 95: Java_nsk_jvmti_GetObjectMonitorUsage_objmonusage001_check(JNIEnv *env, > > I would have expected a modification to an objmonusage001.java file to update > the parameters to that test's `check()` function. The parameters of the `check()` method are named short. It is why I decided to skip this update. Now, I decided to rename parameters to keep them consistent with the other test `objmonusage003`. So, it has been fixed now, thank you for the suggestion. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1482714109
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v3]
On Wed, 7 Feb 2024 22:43:36 GMT, Daniel D. Daugherty 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 three additional
>> commits since the last revision:
>>
>> - Merge
>> - review: thread in notify waiter list can't be BLOCKED
>> - 8324677: Specification clarification needed for JVM TI
>> GetObjectMonitorUsage
>
> test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage001/objmonusage001.cpp
> line 161:
>
>> 159:
>> 160: if (inf.notify_waiter_count != notifyWaiterCount) {
>> 161: printf("(%d) waiter_count expected: %d, actually: %d\n",
>
> nit typo: s/waiter_count/notify_waiter_count/
Nice catch, thanks!
> test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage003/objmonusage003.cpp
> line 150:
>
>> 148:
>> 149: if (inf.notify_waiter_count != notifyWaiterCount) {
>> 150: printf("(%d) waiter_count expected: %d, actually: %d\n",
>
> nit typo: s/waiter_count/notify_waiter_count/
Nice catch, thanks!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1482702600
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1482702844
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v3]
On Wed, 7 Feb 2024 21:56:01 GMT, Daniel D. Daugherty 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 three additional >> commits since the last revision: >> >> - Merge >> - review: thread in notify waiter list can't be BLOCKED >> - 8324677: Specification clarification needed for JVM TI >> GetObjectMonitorUsage > > src/hotspot/share/prims/jvmtiEnvBase.cpp line 1489: > >> 1487: nWait = mon->waiters(); // # of threads in Object.wait() >> 1488: ret.waiter_count = nWant; >> 1489: ret.notify_waiter_count = nWait; > > Please note that the comment on L1487 is accurate. The `waiters` count is > incremented just before the thread that has called `wait()` drops the monitor > and that increased count remains in effect until after the thread has > reentered > the monitor after being notified or the wait timeout has expired. > > The `contentions` count is not incremented in the re-entry path so a thread > that is in `wait()` that has been notified or the wait timeout has expired is > not > counted as a contending thread. > > So if we really want the `waiter_count` field to include threads that have > been > notified or the wait timeout has expired, then we have to add some logic to > carefully increment the `contentions` count. > > This old logic: > ```ret.waiter_count = nWant + nWait;``` > over counts because it also includes threads in `wait()` that have not yet > been notified and the wait timeout has not expired. However, including > `nWait` is correct for the situation when all of the waiting threads have > been notified or their wait timeouts have expired. > > This also means that `nWait` and the `ret.notify_waiter_count` value are > over counting waiters because that value will include threads that (have > been notified or the wait timeout has expired) AND have not reentered > the monitor. I _think_ we're saying that is NOT what we want. However, > I _think_ that the `nWait` value is fixed below when we're gathering up > the list of waiters. Thank you for reviewing this, Dan! > The contentions count is not incremented in the re-entry path so a thread that is in wait() that has been notified or the wait timeout has expired is not counted as a contending thread. Right. I've discovered this after reading your first comment above this comment. :) I agree with the whole comment. I understand this over counting now. It is a good catch! Will work on fixing this. - PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1482696229
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v3]
On Wed, 7 Feb 2024 07:02:11 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with 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 three additional
> commits since the last revision:
>
> - Merge
> - review: thread in notify waiter list can't be BLOCKED
> - 8324677: Specification clarification needed for JVM TI
> GetObjectMonitorUsage
Based on Dan's analysis I would have to go back and redo the complete analysis
for myself. :( I think the only way to make sense of this is to actually set up
scenarios where we have different threads contending on entry, different
threads waiting and different threads re-entering after being notified, and see
what values actually get returned in each case.
I think the `pending_current_monitor` issue may be a distinct/different issue.
I can easily imagine that this was overlooked when we introduced the "wait
morphing" rather than have the notified/timed-out/interrupted waiters actually
place themselves on the entry queue.
-
PR Comment: https://git.openjdk.org/jdk/pull/17680#issuecomment-1933469232
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v3]
On Wed, 7 Feb 2024 07:02:11 GMT, Serguei Spitsyn wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> The Release-Note is:
>> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
>> incorrect implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with 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 three additional
> commits since the last revision:
>
> - Merge
> - review: thread in notify waiter list can't be BLOCKED
> - 8324677: Specification clarification needed for JVM TI
> GetObjectMonitorUsage
Okay... so we might have an incorrect implementation of JVM TI
GetObjectMonitorUsage, but I'm not convinced that it is broken
in the way that we think it is broken.
Please read thru my unfortunately long comments on the complicated
logic in `JvmtiEnvBase::get_object_monitor_usage()`. I _think_ things
are broken in a different way than what we're talking about in this bug.
In particular, the logic that populates the `waiters` array and reduces
the `waiter_count` value overrides the value that we think we're fixing
in the beginning of the function.
Similarly, the logic that populates the `notify_waiters` array and reduces
the `notify_waiter_count` value also overrides the value that we think
we're fixing in the beginning of the function.
src/hotspot/share/prims/jvmtiEnvBase.cpp line 1489:
> 1487: nWait = mon->waiters(); // # of threads in Object.wait()
> 1488: ret.waiter_count = nWant;
> 1489: ret.notify_waiter_count = nWait;
Please note that the comment on L1487 is accurate. The `waiters` count is
incremented just before the thread that has called `wait()` drops the monitor
and that increased count remains in effect until after the thread has reentered
the monitor after being notified or the wait timeout has expired.
The `contentions` count is not incremented in the re-entry path so a thread
that is in `wait()` that has been notified or the wait timeout has expired is
not
counted as a contending thread.
So if we really want the `waiter_count` field to include threads that have been
notified or the wait timeout has expired, then we have to add some logic to
carefully increment the `contentions` count.
This old logic:
```ret.waiter_count = nWant + nWait;```
over counts because it also includes threads in `wait()` that have not yet
been notified and the wait timeout has not expired. However, including
`nWait` is correct for the situation when all of the waiting threads have
been notified or their wait timeouts have expired.
This
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v3]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> The Release-Note is:
> [8325314](https://bugs.openjdk.org/browse/JDK-8325314): Release Note:
> incorrect implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with 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 three additional
commits since the last revision:
- Merge
- review: thread in notify waiter list can't be BLOCKED
- 8324677: Specification clarification needed for JVM TI GetObjectMonitorUsage
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/abe82a6c..3d735722
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=02
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=01-02
Stats: 7346 lines in 273 files changed: 4294 ins; 1478 del; 1574 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v2]
> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
> spec.
> The function returns the following structure:
>
>
> typedef struct {
> jthread owner;
> jint entry_count;
> jint waiter_count;
> jthread* waiters;
> jint notify_waiter_count;
> jthread* notify_waiters;
> } jvmtiMonitorUsage;
>
>
> The following four fields are defined this way:
>
> waiter_count [jint] The number of threads waiting to own this monitor
> waiters [jthread*] The waiter_count waiting threads
> notify_waiter_count [jint] The number of threads waiting to be notified by
> this monitor
> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
> notified
>
> The `waiters` has to include all threads waiting to enter the monitor or to
> re-enter it in `Object.wait()`.
> The implementation also includes the threads waiting to be notified in
> `Object.wait()` which is wrong.
> The `notify_waiters` has to include all threads waiting to be notified in
> `Object.wait()`.
> The implementation also includes the threads waiting to re-enter the monitor
> in `Object.wait()` which is wrong.
> This update makes it right.
>
> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to keep
> the existing behavior of this command.
>
> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
> `GetObjectMonitorUsage()` correct behavior:
>
> jvmti/GetObjectMonitorUsage/objmonusage001
> jvmti/GetObjectMonitorUsage/objmonusage003
>
>
> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>
>
>
> A JCK bug will be filed and the tests have to be added into the JCK problem
> list located in the closed repository by a separate sub-task before
> integration of this update.
>
> Also, please see and review the related CSR:
> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
> implementation of JVM TI GetObjectMonitorUsage
>
> Testing:
> - tested with mach5 tiers 1-6
Serguei Spitsyn has updated the pull request incrementally with one additional
commit since the last revision:
review: thread in notify waiter list can't be BLOCKED
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17680/files
- new: https://git.openjdk.org/jdk/pull/17680/files/80225516..abe82a6c
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=17680=01
- incr: https://webrevs.openjdk.org/?repo=jdk=17680=00-01
Stats: 8 lines in 1 file changed: 0 ins; 7 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/17680.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17680/head:pull/17680
PR: https://git.openjdk.org/jdk/pull/17680
Re: RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage
On Fri, 2 Feb 2024 02:22:54 GMT, David Holmes wrote:
>> The implementation of the JVM TI `GetObjectMonitorUsage` does not match the
>> spec.
>> The function returns the following structure:
>>
>>
>> typedef struct {
>> jthread owner;
>> jint entry_count;
>> jint waiter_count;
>> jthread* waiters;
>> jint notify_waiter_count;
>> jthread* notify_waiters;
>> } jvmtiMonitorUsage;
>>
>>
>> The following four fields are defined this way:
>>
>> waiter_count [jint] The number of threads waiting to own this monitor
>> waiters [jthread*] The waiter_count waiting threads
>> notify_waiter_count [jint] The number of threads waiting to be notified by
>> this monitor
>> notify_waiters [jthread*] The notify_waiter_count threads waiting to be
>> notified
>>
>> The `waiters` has to include all threads waiting to enter the monitor or to
>> re-enter it in `Object.wait()`.
>> The implementation also includes the threads waiting to be notified in
>> `Object.wait()` which is wrong.
>> The `notify_waiters` has to include all threads waiting to be notified in
>> `Object.wait()`.
>> The implementation also includes the threads waiting to re-enter the monitor
>> in `Object.wait()` which is wrong.
>> This update makes it right.
>>
>> The implementation of the JDWP command `ObjectReference.MonitorInfo (5)` is
>> based on the JVM TI `GetObjectMonitorInfo()`. This update has a tweak to
>> keep the existing behavior of this command.
>>
>> The follwoing JVMTI vmTestbase tests are fixed to adopt to the
>> `GetObjectMonitorUsage()` correct behavior:
>>
>> jvmti/GetObjectMonitorUsage/objmonusage001
>> jvmti/GetObjectMonitorUsage/objmonusage003
>>
>>
>> The following JVMTI JCK tests have to be fixed to adopt to correct behavior:
>>
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00101/gomu00101a.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102.html
>> vm/jvmti/GetObjectMonitorUsage/gomu001/gomu00102/gomu00102a.html
>>
>>
>>
>> A JCK bug will be filed and the tests have to be added into the JCK problem
>> list located in the closed repository by a separate sub-task before
>> integration of this update.
>>
>> Also, please see and review the related CSR:
>> [8324677](https://bugs.openjdk.org/browse/JDK-8324677): incorrect
>> implementation of JVM TI GetObjectMonitorUsage
>>
>> Testing:
>> - tested with mach5 tiers 1-6
>
> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1553:
>
>> 1551: Handle th(current_thread, get_vthread_or_thread_oop(w));
>> 1552: if (java_lang_Thread::get_thread_status(w->threadObj()) ==
>> 1553: JavaThreadStatus::BLOCKED_ON_MONITOR_ENTER) {
>
> I don't think this is possible. `BLOCKED_ON_MONITOR_ENTER` is only set after
> the target has been removed from the wait-set and added to the cxq queue -
> see `ObjectMonitor::INotify`. As per the comment just above:
>
> // If the thread was found on the ObjectWaiter list, then
> // it has not been notified.
>
> which means it can't be trying to acquire the monitor either.
Thank you for looking at it and the comment, David! Will check what is possible
here.
Do you have any suggestions?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1477954550
