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

Tue Oct 1 23:57:16 UTC 2019

Hi David,

Yes, this is another place to fix the same typo, thanks.
It has to be results[i] instead of err.
I'll update the webrev in place.

Thanks,
Serguei

On 10/1/19 4:00 PM, David Holmes wrote:
> 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>