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

David Holmes david.holmes at oracle.com
Wed Oct 2 02:27:51 UTC 2019 

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
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>

More information about the serviceability-dev mailing list