Hi David, Thanks for the review, the new webrev is hosted at
http://cr.openjdk.java.net/~cl/host_for_kal/6772009-CancelledLockLoop/
. Please see inline text.
On 11/20/13, 6:35 PM, David Holmes wrote:
On 21/11/2013 10:28 AM, Martin Buchholz wrote:
I again tried and failed to reproduce a failure. Even if I go
whole
hog
and multiply TIMEOUT by 100 and divide ITERS by 100, the test
continues to
PASS. Is it just me?!
I think you are going the wrong way Martin - you want the timeout to
be smaller than the time it takes to execute ITERS.
I don't think there's any reason to make result long. It's not
even
used
except to inhibit hotspot optimizations.
+ private volatile long result = 17;//Can get int
overflow,so
using long
Further the subsequent use of += is incorrect as it is not an atomic
operation. Even if we don't care about the value, it looks bad.
Made the necessary changes for atomic update.
I'm not sure result must be updated if we get a
BrokenBarrierException either. Probably harmless, but necessary?
I retained it in the fix for completeness in updating the numbers,
please let me know if you still think otherwise.
need to fix spelling and spacing below.
+ barrier.await();//If a BrokeBarrierException
happens
here(due to
There are a number of style issues with spacing around comments.
Fixed the spelling error and styling issues.
And I don't think this change is sufficient to claim co-author
status
with Doug either ;-)
Removed the claim :)
The additional tracing may be useful but seems stylistically
different from the rest of the code.
Retained the tracking to understand if it is again the timing issue
which is the cause in an event of a failure, however i can remove it
if you think it is not necessary (OR) include an alternate
solution as
you may want to suggest.
Overall I'm suspicious that the changed timeout actually fixes
anything - normally we need to add longer timeouts not shorter ones.
Does this fail on a range of machines or only specific ones? Have we
verified that the clocks/timers are behaving properly on those
systems?
Here the time out is not about waiting for threads to complete
something but to "be interrupted" before being considered done, so we
decreased the timeout. However we now chose to increase the number of
iterations to 5000000 from 1000000(thanks to tristan for the
suggestion) instead of decreasing the timeout as done earlier because
the increasing iterations ensures the threads are busy for long time
curtailing the need to touch the timeout.
Thanks,
David
--
Thanks
kalyan
Ph: (408)-585-8040
On Wed, Nov 20, 2013 at 11:52 AM, srikalyan <
srikalyan.chandrashe...@oracle.com> wrote:
Hi Martin , apologies for the delay , was trying to get help for
hosting
my webrev. . Please see inline text.
On 11/19/13, 10:35 PM, Martin Buchholz wrote:
Hi Kalyan,
None of us can review your changes yet because you haven't given
us a
URL of your webrev.
It is located here
http://cr.openjdk.java.net/~cl/host_for_srikalyan_6772009_CancelledLockLoops/
I've tried to make the jsr166 copy of CancelledLockLoops fail by
adjusting ITERS and TIMEOUT radically up and down, but the test
just keeps
on passing for me. Hints appreciated.
Bump up the timeout to 500ms and you will see a failure (i can
see it
consistently on my machine Linux 64bit,8GBRAM,dual cores, with
JDK 1.8
latest any promoted build).
On Tue, Nov 19, 2013 at 6:39 PM, srikalyan chandrashekar <
srikalyan.chandrashe...@oracle.com> wrote:
Suggested Fix:
a) Decrease the timeout from 100 to 50ms which will ensure that
the test
will pass even on faster machines
This doesn't look like a permanent fix - it just makes the
failing case
rarer.
Thats true , the other way is to make the main thread wait on
TIMEOUT
after firing the interrupts instead of other way round, but that
would be
over-optimization which probably is not desirable as well. The 50
ms was
arrived at empirically after running several 100 times on multiple
configurations and did not cause failures.
--
Thanks
kalyan
Ph: (408)-585-8040
