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

Wed Jan 22 05:07:30 PST 2014

Updated webrev with no "arrive()" calls dangling.

- http://cr.openjdk.java.net/~jbachorik/6309226/webrev.07

-JB-

On 22.1.2014 09:12, Jaroslav Bachorik wrote:
> 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-
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>>
>