Seems good to go to me.
David
On 5/09/2013 10:52 PM, Jaroslav Bachorik wrote:
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-
