RFR: 8346120: VirtualThreadPinned event recorded for Object.wait may have wrong duration or may record second event [v3]

[David Holmes]

On Sat, 14 Dec 2024 16:27:19 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> A jdk.VirtualPinnedEvent JFR event is recorded by Object::wait when a virtual thread waits in Object.wait while pinned. The posting of the event in ObjectMonitor::wait is done after waiting but it can block again in enter/ReenterI before it re-acquires. If that happens then the event duration won't be accurate, and for the off-list case, the enter may be contended and a jdk.VirtualPinnedEvent event will be recorded for contended monitor enter. The changes to fix this are small: move the post of the event for Object.wait to end of the wait function, and only post an event for contended monitor enter when not waiting.
>> 
>> I've added a test to check the pinned event that is recored for several contended monitor enter and Object.wait cases. It checks the event duration and that only one event is recorded for each case.
>
> Alan Bateman has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision:
> 
>  - Merge branch 'master' into JDK-8346120
>  - Fix test comments
>  - testObjectWait2 test already owns lock
>  - Min duration check needs to have some tolerance
>  - Rename test, check more event fields
>  - Filter out VirtualThread.getAndClearInterrupt
>  - Initial commit

So to be clear ... you want one pinned event, the duration of which spans the wait and the monitor reentry. If we are pinned for the wait then we must be pinned for the reentry for the same reason, so no need to report the same thing twice.

src/hotspot/share/runtime/objectMonitor.cpp line 1864:

> 1862: 
> 1863:   if (ce != nullptr && ce->is_virtual_thread()) {
> 1864:     assert(result != freeze_ok, "sanity check");

This assertion is so far from where `result` is set that I don't think it serves any purpose. We would have returned at line 1693 already. It was already a long way before you moved this.

-------------

PR Review: https://git.openjdk.org/jdk/pull/22718#pullrequestreview-2504712841
PR Review Comment: https://git.openjdk.org/jdk/pull/22718#discussion_r1885866755