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

Wed Jan 22 00:12:06 PST 2014

Argh. Sorry. Probably messed up when restoring my crashed HDD. I will go through the patch to see if there are any other omissions.

Thanks,
-JB-

David Holmes <david.holmes at oracle.com> wrote:
>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-
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>>

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20140122/4bf68bbd/attachment-0001.html 