On Wed, 15 May 2024 20:09:52 GMT, Serguei Spitsyn <sspit...@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
