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

List:       openjdk-serviceability-dev
Subject:    Re: RFR 6309226: TEST: java/lang/management/ThreadMXBean/SynchronizationStatistics.java
From:       Jaroslav Bachorik <jaroslav.bachorik () oracle ! com>
Date:       2013-12-20 8:24:30
Message-ID: 52B3FEBE.40106 () oracle ! com
[Download RAW message or body]

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

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