SynchronizationStatistics.java didn't check Thread.sleep (original) (raw)

Jaroslav Bachorik jaroslav.bachorik at oracle.com
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/jmx-dev/attachments/20140122/4bf68bbd/attachment.html