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

[David Holmes]

Hi Jaroslav,

I still see some suspect uses of p.arrive() instead of 
p.arriveAndAwaitAdvance().

David

On 21/01/2014 11:14 PM, Jaroslav Bachorik wrote:
> 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-
>>>>>>>>
>>>>>>
>>>>
>>
>