RFR (M): 8231595 [TEST] develop a test case for SuspendThreadList including current thread

[Chris Plummer]

Hi Serguei,

If someone changes THREADS_COUNT, then SuspenderIndex would no longer do 
what we want. I suggest passing in something like "first" and "last".

  120      *  - main thread registers tested threads withing the native 
agent library

Should be "within".

I think you should add a comment above all the "n" and "i" logic 
explaining what it is for, although TBH, I don't see how this is making 
the method "hot" and therefore trigger compilation. The loop alone 
should be enough for that. In fact I would think the more iterations 
through the loop, the sooner it would be compiled, and this extra logic 
just slows down the iteration rate.

thanks,

Chris

On 9/30/19 10:45 PM, serguei.spitsyn at oracle.com wrote:
> Hi Chris, David and Alex,
>
> The updated webrev is:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8231595-jvmti-susp-tlist.2/ 
>
>
>
> It includes the changes:
>   - added a general comment explaining the test logic suggested by David
>   - setAllThreadsReady() is static now, the obsolete comment before it 
> is deleted
>   - now the test is run twice: with the suspender thread first and 
> last in the list
>   - removed the local variable "success" in the run() method
>   - several native agent methods return "void" now
>
> Thanks,
> Serguei
>
>
> On 9/30/19 13:49, Chris Plummer wrote:
>> On 9/30/19 1:30 PM, serguei.spitsyn at oracle.com wrote:
>>> On 9/30/19 13:25, Chris Plummer wrote:
>>>> On 9/30/19 1:21 PM, serguei.spitsyn at oracle.com wrote:
>>>>> Hi Chris,
>>>>>
>>>>> Thank you for reviewing this!
>>>>>
>>>>>
>>>>> On 9/28/19 12:33, Chris Plummer wrote:
>>>>>> Hi Serguei,
>>>>>>
>>>>>> Overall looks good. A few questions:
>>>>>>
>>>>>> I don't understand the need for all the 'i' and 'n' theatrics in 
>>>>>> the shouldFinish loop. Can you explain and also add a comment?
>>>>>
>>>>> I used this part from one of the old SuspendThreadList nsk tests 
>>>>> in the vmTestbase.
>>>>> As I understand it, the point was to make sure the JVMTI 
>>>>> SuspendThreadList works well
>>>>> wen the top frames executed on tested threads have been compiled.
>>>>> These code is needed to make the run() method hot.
>>>>> I can add a comment if you think it is not clear.
>>>>>
>>>>>
>>>>>>
>>>>>> Is this comment right?
>>>>>>
>>>>>>  193     // set thread is not ready again
>>>>>>  194     public void setAllThreadsReady() {
>>>>>>  195         allThreadsReady = true;
>>>>>>  196     }
>>>>>
>>>>> Nice catch.
>>>>> The comment is not needed anymore.
>>>>> Is a leftover from previous test version.
>>>>>
>>>>>>
>>>>>> Also, shouldn't "setAllThreadsReady()" be static?
>>>>>
>>>>> Right. It has to be static. Will fix it.
>>>>>
>>>>>
>>>>>> Do you think it would be useful to also run the test with the 
>>>>>> last thread in the list being the suspender thread?
>>>>>
>>>>> I'm not sure it is worth to do it.
>>>>> It'd add more complexity into the test.
>>>>> We could try to make the suspender thread to be random though.
>>>> I don't think random is good. Makes it hard to reproduce an issue 
>>>> if it turns up. I was thinking just rerun the test for each 
>>>> possible suspender.
>>>
>>> Good idea to rerun the test and pass the suspender thread index in 
>>> the arguments.
>>> Do you think, two runs would be good enough or we also need an index 
>>> somewhere in the middle?
>> Maybe just first and last indices would be good. I'm not sure 
>> something in the middle helps any.
>>
>> Chris
>>>
>>> Thanks,
>>> Serguei
>>>
>>>>
>>>> thanks,
>>>>
>>>> Chris
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> Chris
>>>>>>
>>>>>> On 9/27/19 6:25 PM, serguei.spitsyn at oracle.com wrote:
>>>>>>> Please, review fix for test enhancement:
>>>>>>>   https://bugs.openjdk.java.net/browse/JDK-8231595
>>>>>>>
>>>>>>> Webrev:
>>>>>>> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8231595-jvmti-susp-tlist.1/ 
>>>>>>>
>>>>>>>
>>>>>>> Summary:
>>>>>>>   New test is a coverage for the JVMTI bug:
>>>>>>>     https://bugs.openjdk.java.net/browse/JDK-8217762
>>>>>>>       SuspendThreadList won't work correctly if the current 
>>>>>>> thread is not last in the list
>>>>>>>
>>>>>>>   It provides a prove the bug JDK-8217762 does not exist
>>>>>>>   as the test is passed with the current implementation.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Serguei
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>