Re: RFR: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner [v2]

2024-04-16 Thread Serguei Spitsyn 
On Tue, 16 Apr 2024 19:21:16 GMT, Leonid Mesnik  wrote:

> The fix looks good, just one nit. rml.wait(100); might be changed to plain 
> sleep(100)

Right, thanks. Will fix it.

-

PR Comment: https://git.openjdk.org/jdk/pull/18778#issuecomment-2059950982

Re: RFR: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner [v2]

2024-04-16 Thread Leonid Mesnik 
On Tue, 16 Apr 2024 06:08:58 GMT, Serguei Spitsyn  wrote:

>> This is the test issue. The `WaitingPT3` thread posted the `MonitorWait` 
>> event but has not released the `lockCheck` monitor yet. It has been fixed to 
>> wait for each `WaitingTask` thread to really reach the `WAITING` state. The 
>> same approach is used for `EnteringTask` threads. It has been fixed to wait 
>> for each `EnteringTask` thread to reach the `BLOCKED_ON_MONITOR` state.
>> 
>> Testing:
>>  - Reran the test 
>> `serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java` locally
>>  - TBD: submit the mach5 tiers 1-6 as well
>
> Serguei Spitsyn has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   review: removed event_lock; moved wait_for_state() to jvmti_common.hpp

Marked as reviewed by lmesnik (Reviewer).

The fix looks good, just one nit.
 rml.wait(100); might be changed to plain sleep(100)

-

PR Review: https://git.openjdk.org/jdk/pull/18778#pullrequestreview-2004437310
PR Comment: https://git.openjdk.org/jdk/pull/18778#issuecomment-2059776412

Re: RFR: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner [v2]

2024-04-16 Thread Serguei Spitsyn 
> This is the test issue. The `WaitingPT3` thread posted the `MonitorWait` 
> event but has not released the `lockCheck` monitor yet. It has been fixed to 
> wait for each `WaitingTask` thread to really reach the `WAITING` state. The 
> same approach is used for `EnteringTask` threads. It has been fixed to wait 
> for each `EnteringTask` thread to reach the `BLOCKED_ON_MONITOR` state.
> 
> Testing:
>  - Reran the test 
> `serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java` locally
>  - TBD: submit the mach5 tiers 1-6 as well

Serguei Spitsyn has updated the pull request incrementally with one additional 
commit since the last revision:

  review: removed event_lock; moved wait_for_state() to jvmti_common.hpp

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18778/files
  - new: https://git.openjdk.org/jdk/pull/18778/files/1a1cde71..9941c492

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18778=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=18778=00-01

  Stats: 38 lines in 2 files changed: 18 ins; 18 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/18778.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18778/head:pull/18778

PR: https://git.openjdk.org/jdk/pull/18778

Re: RFR: 8328741: serviceability/jvmti/ObjectMonitorUsage/ObjectMonitorUsage.java failed with unexpected owner [v2]

2024-04-16 Thread Serguei Spitsyn 
On Mon, 15 Apr 2024 21:34:48 GMT, Leonid Mesnik  wrote:

>> Serguei Spitsyn has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   review: removed event_lock; moved wait_for_state() to jvmti_common.hpp
>
> test/hotspot/jtreg/serviceability/jvmti/ObjectMonitorUsage/libObjectMonitorUsage.cpp
>  line 183:
> 
>> 181: 
>> 182: static void wait_for_state(JNIEnv *jni, jthread thread, jint exp_state) 
>> {
>> 183:   RawMonitorLocker rml(jvmti, jni, event_lock);
> 
> The event_lock name is misleading, there are no events anymore.
> Also, I am not sure if this lock is needed at all. How it is used? 
> 
> I think that wait_for_state is a good candidate to be added in the library. 
> With some additional doc about states.

Good suggestions, thanks. Implemented now.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18778#discussion_r1566755184