On Mon, 4 Mar 2024 03:41:55 GMT, David Holmes <dhol...@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
