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 <[email protected]> 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.
