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 [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 [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&pr=17680&range=07
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17680&range=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
