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

[David Holmes]

Hi Serguei,

Shouldn't this:

  80   for (int i = 0; i < threadsCount; i++) {
   81     LOG("  thread #%d: (%d)", i, (int)results[i]);
   82     check_jvmti_status(jni, err, "suspendTestedThreads: error in 
SuspendThreadList");

also be testing results[i] rather than err? Or do you need to test err 
independently, as well as each result?

David

On 2/10/2019 8:33 am, serguei.spitsyn at oracle.com wrote:
> Alex, Chris and David,
> 
> The updated webrev is:
> http://cr.openjdk.java.net/~sspitsyn/webrevs/2019/8231595-jvmti-susp-tlist.3/ 
> 
> 
> This version changes are:
>    - the "first" and "last" are passed to the test to set the 
> suspenderIndex
>    - fixed typo at line 120
>    - the ThreadToSuspend.run() loop is simplified
>    - fixed typo in check_jvmti_status in the loop of resumeTestedThreads()
> 
> Probably, just one sanity check would be enough.
> 
> Thanks,
> Serguei
> 
> 
> On 10/1/19 2:20 PM, serguei.spitsyn at oracle.com wrote:
>> Hi Chris,
>>
>> On 10/1/19 12:46 PM, Chris Plummer wrote:
>>> 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".
>>
>> Okay, I'll update it this way.
>>
>>>  120 *  - main thread registers tested threads withing the native 
>>> agent library
>>>
>>> Should be "within".
>>
>> Good catch, will fix it.
>>
>>> 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.
>>
>> Agreed.
>> I'll try to simplify it.
>>
>> Thanks,
>> Serguei
>>
>>
>>> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>