RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v23]
Daniel D. Daugherty
dcubed at openjdk.org
Mon Mar 11 18:56:28 UTC 2024
On Fri, 8 Mar 2024 06:11:23 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> 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<JavaThread*>* 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: vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage002/TestDescription.java \
> 972: vmTestbase/nsk/jvmti/GetObjectMonitorUsage/objmonusage003/TestDescription.java \
This file needs a copyright year update.
test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/libObjectMonitorUsage.cpp line 79:
> 77: waits_to_be_notified--;
> 78: }
> 79: }
I like these for getting rid of the raciness of knowing when a thread is blocked
on contention, finished entering, blocked on wait or finished waiting. Nice!
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.
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?
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.
-------------
Marked as reviewed by dcubed (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17680#pullrequestreview-1928679264
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520200433
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520203806
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520208119
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520213205
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520271891
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520264870
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520266194
PR Review Comment: https://git.openjdk.org/jdk/pull/17680#discussion_r1520235253
More information about the hotspot-dev
mailing list