RFR 6309226: TEST: java/lang/management/ThreadMXBean/SynchronizationStatistics.java didn't check Thread.sleep
David Holmes
david.holmes at oracle.com
Wed Jan 22 22:06:04 PST 2014
On 22/01/2014 11:07 PM, Jaroslav Bachorik wrote:
> Updated webrev with no "arrive()" calls dangling.
>
> - http://cr.openjdk.java.net/~jbachorik/6309226/webrev.07
Thanks looks okay. The proof as always is in the running of the test.
David
> -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-
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>
>
More information about the serviceability-dev
mailing list