[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR (M): 8231595 [TEST] develop a test case for SuspendThreadList including current thread
From:       "serguei.spitsyn () oracle ! com" <serguei ! spitsyn () oracle ! com>
Date:       2019-09-30 20:58:27
Message-ID: ddea1678-8d3f-faf3-0cc9-41db9027d616 () oracle ! com
[Download RAW message or body]

On 9/30/19 13:49, Chris Plummer wrote:
> On 9/30/19 1:30 PM, serguei.spitsyn@oracle.com wrote:
>> On 9/30/19 13:25, Chris Plummer wrote:
>>> On 9/30/19 1:21 PM, serguei.spitsyn@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.

Agreed, thanks!
Serguei


>
> Chris
>>
>> Thanks,
>> Serguei
>>
>>>
>>> thanks,
>>>
>>> Chris
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>>
>>>>> thanks,
>>>>>
>>>>> Chris
>>>>>
>>>>> On 9/27/19 6:25 PM, serguei.spitsyn@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
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>

[prev in list] [next in list] [prev in thread] [next in thread]