On Tue, 14 May 2024 17:51:03 GMT, Chris Plummer <cjplum...@openjdk.org> wrote:
>>> expEnteringCount/expWaitingCount contain the tested patterns.
>>
>> I kind of disagree.
>> Please, take look at the loop below:
>>
>> for (int i = 0; i < NUMBER_OF_WAITING_THREADS; i++) {
>> expEnteringCount = isVirtual ? 0 :
>> NUMBER_OF_ENTERING_THREADS + i + 1;
>> expWaitingCount = isVirtual ? 0 : NUMBER_OF_WAITING_THREADS
>> - i - 1;
>> lockCheck.notify(); // notify waiting threads one by one
>> // now the notified WaitingTask has to be blocked on the
>> lockCheck re-enter
>>
>> // entry count: 1
>> // count of threads waiting to enter:
>> NUMBER_OF_ENTERING_THREADS
>> // count of threads waiting to re-enter: i + 1
>> // count of threads waiting to be notified:
>> NUMBER_OF_WAITING_THREADS - i - 1
>> check(lockCheck, expOwnerThread(), expEntryCount(),
>> expEnteringCount,
>> expWaitingCount);
>> }
>>
>> The comment fixed as you suggest does not look useful anymore as the tested
>> pattern is lost:
>>
>> // entry count: expOwnerThread()
>> // count of threads waiting to enter: expEnteringCount
>> // count of threads waiting to re-enter: expEntryCount()
>> // count of threads waiting to be notified: expWaitingCount
>> check(lockCheck, expOwnerThread(), expEntryCount(),
>> expEnteringCount,
>> expWaitingCount);
>> }
>>
>>
>> I understand your concern but your suggestion is not that good.
>> We could remove these comments but the tested pattern will be thrown away
>> with the comments.
>> Would it help if we add clarifications that the comments are correct for
>> platform threads only?
>
> I don't understand the issue with the updated commented. It is precisely
> telling you what the expected "count" values should be. If you leave the
> macros in the comment, then the comment is wrong for virtual threads. If you
> want to keep the macros in the comment, you need to add something like "...
> or 0 for virtual threads".
>
> BTW, the "re-enter" comment should continue to be "i + 1". I'm not sure why
> it was changed to "expEntryCount()".
Okay, please, let me explain this one more time.
The original comments before method `check()` calls describe the testing
scenario but not the numbers expected to be returned by the JVMTI
`GetObjectMonitorUsage`.
For instance, if the testing scenario says: "count of threads waiting to enter:
NUMBER_OF_ENTERING_THREADS" then it means there is a real number of these
threads waiting to enter the monitor. And it does not matter if they are
platform or virtual threads. They are really waiting to enter the monitor.
However, the JVMTI `GetObjectMonitorUsage` won't include virtual threads into
the returned results.
Now, I'm suggesting to add the following header for comments before each
`check()` method call:
+ // The numbers below describe the testing scenario, not the
expected results.
+ // The expected numbers are different for virtual threads because
+ // they are not supported by JVMTI GetObjectMonitorUsage.
Would it work for you?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19030#discussion_r1600769143
