RFR 6309226: TEST: java/lang/management/ThreadMXBean/SynchronizationStatistics.java didn't check Thread.sleep

[Mandy Chung]

Hi Jaroslav,

This is a good cleanup to make the test more reliable.  Thanks for doing 
that.  I agree with David that it could be possible for the current 
thread to arrive at the phaser two times at the same phase while the 
test expects both threads move to the next step when both arrive.  
CyclicBarrier per phase is a good suggestion.

Nits: testBlocked() has some debugging code that can be removed. It'd be 
good to add more comments to describe the individual test case.

thanks
Mandy

On 11/18/2013 1:03 PM, David Holmes wrote:
> Hi Jaroslav,
>
> I think your phaser usage is incorrect:
>
>  88             public void run() {
>   89                 p.arriveAndAwaitAdvance(); // phase[1]
>   90                 synchronized(lock1) {
>   91                     System.out.println("[LockerThread obtained 
> Lock1]");
>   92                     p.arrive(); // phase[2]
>   93                 }
>   94                 p.arriveAndAwaitAdvance(); // phase[3]
>   95             }
>
> Here the current thread can release itself at line 94. You have 
> assumed that the phase[2] arrive will be a trigger to release the main 
> thread but it may not have reached its arriveAndAwaitAdvance phase[2] 
> statement yet, so the current thread arrives then does arrive-and-wait 
> but the number of arrivals is 2 so it doesn't wait.
>
> A CyclicBarrier per phase may be clearer.
>
> David
>
> On 18/11/2013 7:59 PM, Jaroslav Bachorik wrote:
>> Hi,
>>
>> after discussing this with Mandy I've rewritten the test to use the
>> j.u.concurrent for synchronization - this also makes it much easier to
>> follow the test logic.
>>
>> The waited time, the blocked time and the waited counts are only checked
>> for sanity (increasing values) since it is not possible to do the
>> reliable checks against hard numbers.
>>
>> I ran the test in a tight loop for 1500 times using -Xcomp and -Xint and
>> the test seems to pass constantly.
>>
>> New webrev: http://cr.openjdk.java.net/~jbachorik/6309226/webrev.03
>>
>> Thanks,
>>
>> -JB-
>>
>>
>> On 21.10.2013 13:55, Jaroslav Bachorik wrote:
>>> Please, review this small patch for a test failing due to the updated
>>> implementation in JDK6.
>>>
>>> Issue:  https://bugs.openjdk.java.net/browse/JDK-6309226
>>> Webrev: http://cr.openjdk.java.net/~jbachorik/6309226/webrev.00/
>>>
>>> The test fails due to the change in mustang where
>>> ThreadMXBean.getThreadInfo().getWaitedTime() and
>>> ThreadMXBean.getThreadInfo().getWaitedCount() include Thread.sleep()
>>> too. Unfortunately, Thread.sleep() is used throughout the test for
>>> synchronization purposes and this breaks the test.
>>>
>>> In the patch I propose to replace Thread.sleep() with busy wait and
>>> hinting the scheduler by Thread.yield(). While not very elegant it
>>> successfully works around inclusion of unknown number of 
>>> Thread.sleep()s
>>> (they are called in loop).
>>>
>>> Thanks,
>>>
>>> -JB-
>>