On 09/04/2013 10:33 AM, Jaroslav Bachorik wrote: > On 09/04/2013 10:29 AM, David Holmes wrote: >> On 4/09/2013 4:56 PM, Jaroslav Bachorik wrote: >>> On 09/04/2013 04:24 AM, Mandy Chung wrote: >>>> Hi Jaroslav, >>>> >>>> Like Daniel and David said, CyclicBarrier and other j.u.concurrent >>>> utility seem a good replacement with the ThreadExecutionSynchronizer >>>> class. ThreadMXBean/Locks.java was written prior to j.u.concurrent >>>> added to the platform (both java.util.concurrent and >>>> java.lang.management were added in JDK 5). Later >>>> ThreadExecutionSynchronizer was added to fix some intermittent issue. >>>> >>>> I think it's worth the investigation to replace the existing logic with >>>> j.u.concurrent and get rid of ThreadExecutionSynchronizer (which is used >>>> by a few other tests). >>> >>> >>> Ok, let's go back to the basics :) >>> >>> The reason for the test to fail intermittently are stale reads from the >>> "waiting" variable. In order to minimize the changes it seems sufficient >>> to make the "waiting" variable volatile to prevent the stale reads. The >>> code modifying the "waiting" variable is already guarded by the >>> semaphore so we are good there. >> >> Yes that seems valid. >> >>> The changes in Locks.java are about more consistent approach to waiting >>> for a thread to enter a desired state. I took Erik's recommendation to >>> just wait indefinitely for the desired thread state and let the harness >>> deal with timeouts. >> >> Yes that seems good too. >> >>> The very simplified patch is at >>> http://cr.openjdk.java.net/~jbachorik/6815130/webrev.03 >> >> One thing you "fixed" in the original version but is no longer "fixed" >> is findOwnerInfo where you made the break exit both loops. I'm unclear >> if that is a correctness issue or just an optimization? I suspect an >> optimization and that once you have found what you need, >> lock.equals(blockedLock) will not be true for any other ThreadInfo objects. > > Exactly. It was an optimization and I excluded the change to keep the > fix very simple - given that the whole test would very likely be > rewritten for JDK9.
Task for removal of java/lang/management/ThreadMXBean/ThreadExecutionSynchronizer filed - JDK-8024326 So, is this change good to go? Thanks, -JB- > > -JB- > >> >> Thanks, >> David >> >>> I will file a task for JDK9 to remove ThreadExecutionSynchronizer and >>> simplify java.lang.management tests using it. >>> >>> -JB- >>> >>>> >>>> Mandy >>>> >>>> On 9/3/2013 4:15 AM, Jaroslav Bachorik wrote: >>>>> Please, review the following patch of the intermittently failing test: >>>>> http://cr.openjdk.java.net/~jbachorik/6815130/webrev.01 >>>>> >>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-6815130 >>>>> >>>>> >>>>> Sometimes the ThreadExecutionSynchronizer class failes to achieve the >>>>> desired synchronization (due to possible data races when evaluating and >>>>> setting the "waiting" variable) leading to test failures. >>>>> >>>>> The patch fixes the possibility of data race. Also the Locks class is >>>>> tidied up a bit. >>>>> >>>>> Thanks, >>>>> >>>>> -JB- >>>> >>> >
