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

[Jaroslav Bachorik]

Based on the experience while fixing 
https://bugs.openjdk.java.net/browse/JDK-8032377 I've updated the patch 
not to require an exact number for the blocked count - it will pass 
whenever the reported blocked count is at least large as the the 
expected number.

Webrev: http://cr.openjdk.java.net/~jbachorik/6309226/webrev.06

Thanks,

-JB-

On 20.12.2013 09:24, Jaroslav Bachorik wrote:
> On 20.12.2013 03:27, David Holmes wrote:
>> Sorry for the delay - still catching up after vacation (only 435 openjdk
>> emails left :( ).
>
> NP
>
>>
>> On 9/12/2013 9:46 PM, Jaroslav Bachorik wrote:
>>> On 19.11.2013 19:55, David Holmes wrote:
>>>> On 20/11/2013 12:43 AM, Jaroslav Bachorik wrote:
>>>>> Hi David,
>>>>>
>>>>> On 18.11.2013 22:03, 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.
>>>>>
>>>>> Yes, thanks for pointing this out. But, wouldn't
>>>>> p.arriveAndAwaitAdvance() instead of p.arrive() suffice? I've tried to
>>>>> replace Phaser with CyclicBarrier but while it seems to work as
>>>>> expected
>>>>> it pollutes code with the necessity to catch various exceptions
>>>>> (InterruptedException, BarrierException etc.). So, if the simple fix
>>>>> with Phaser would do the trick I would better use that.
>>>>
>>>> In this case yes arriveAndAwaitAdvance would fix it. I don't know if
>>>> other tests have the same issue - the concern would be ensuring there's
>>>> no possibility of deadlocks in general.
>>>
>>> I've updated the webrev with the one using p.arriveAndAwaitAdvance() at
>>> line 92 - http://cr.openjdk.java.net/~jbachorik/6309226/webrev.04
>>>
>>> Are you ok with this change then?
>>
>> I think you have the same problem anywhere that arrive() is used eg:
>>
>>   129                     p.arrive(); // phase[2]
>>   130                     p.arriveAndAwaitAdvance(); // phase[3]
>>
>> and
>>
>>   185                     p.arrive(); // phase[2]
>>   186                 }
>>   187                 p.arriveAndAwaitAdvance(); // phase[3]
>
> Very probable :( Also, when analysing the recurrence of JDK-8029890 I've
> found out that Phaser.arriveAndAwaitAdvance() actually blocked the
> thread in a way that it increased the number of contentions reported :(
> CyclicBarrier seems to wait instead - I will have to use CyclicBarrier
> when testing the number of reported contentions and Phaser when testing
> the number of reported waits :/
>
> -JB-
>
>>
>> David
>> ------
>>
>>> Thanks,
>>>
>>> -JB-
>>>
>>>
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> -JB-
>>>>>
>>>>>>
>>>>>> 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-
>>>>>>>
>>>>>
>>>
>