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

[Jaroslav Bachorik]

Hi David,

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?

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