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

Wed Oct 2 08:52:46 UTC 2019

Thank you for review, David!
Serguei

On 10/1/19 19:27, David Holmes wrote:
> Looks good.
>
> Thanks,
> David
>
> On 2/10/2019 9:57 am, serguei.spitsyn at oracle.com wrote:
>> 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
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>