jmx-dev RFR 6309226: TEST: java/lang/management/ThreadMXBean/SynchronizationStatistics.java didn't check Thread.sleep (original) (raw)

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 jmx-dev mailing list