RFR: 8328083: degrade virtual thread support for GetObjectMonitorUsage [v4]

[Serguei Spitsyn]

On Wed, 15 May 2024 20:09:52 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1535:
>> 
>>> 1533:       bool is_virtual = java_lang_VirtualThread::is_instance(thread_oop);
>>> 1534:       if (is_virtual) {
>>> 1535:         skipped++;
>> 
>> Do we really need to maintain `skipped`. Isn't not adding to `nWait` the same as skipping?
>
> Good suggestion, thanks. Fixed now.

I've undone this suggested simplification as it has not worked out. Please, see my answer on your next comment.

>> src/hotspot/share/prims/jvmtiEnvBase.cpp line 1583:
>> 
>>> 1581:         assert(w != nullptr, "sanity check");
>>> 1582:         if (java_lang_VirtualThread::is_instance(thread_oop)) {
>>> 1583:           skipped++;
>> 
>> I don't think maintaining `skipped` does anything here.
>
> Thank you for the question. It is needed at the line 1586 below to discount the index:
> 
>         if (java_lang_VirtualThread::is_instance(thread_oop)) {
>           skipped++;
>         } else {
>           // If the thread was found on the ObjectWaiter list, then
>           // it has not been notified.
>           Handle th(current_thread, get_vthread_or_thread_oop(w));
> 1586:     ret.notify_waiters[i - skipped] = (jthread)jni_reference(calling_thread, th);
>         }

BTW: The simplification (getting rid of local `skipped`) you requested in previous comment damaged this fragment by making it incorrect. Here we need the `nWait` to account for virtual threads as well. Otherwise, the loop is shorted.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1602212314
PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1602210128