RFR: 8247972: incorrect implementation of JVM TI GetObjectMonitorUsage [v21]

[Serguei Spitsyn]

On Mon, 4 Mar 2024 03:41:55 GMT, David Holmes <dholmes at openjdk.org> 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