As I said in the original e-mail "I think the methods reset and getCount
should be synchronized.".
Either synchronizing both those methods or making _count volatile would
fix the problem. I don't think performance matters on this class, so the
simplest solution should win. The increment method is, and has to be,
synchronized, so we might was well apply the same solution throughout.
If performance did matter, I would still prefer making all the methods
synchronized. Almost all calls are to increment. Making _count volatile
would tend to slow down every call to increment by adding memory
barriers. Making the other methods synchronized only affects increment
in the rare case of contention with one of the others.
As I've worked on the other problem, I've continued to build empirical
evidence that this is a real problem. Since I made the changes to
Counter, I have run the test dozens of times, without seeing the failure
mode in which everything works but the test fails to notice it has
finished. Before the fix, that was the commoner failure mode.
The _count variable is part of a much bigger issue. I've been assuming
that I should not make style fixes as part of bug fixes. Although I
would have called the variable "count" myself, at least it does not get
in the way of being able to read the code. I have more trouble with
inconsistent indentation, to the extent that I sometimes reformat files
I know I will revert before the next check-in.
Maybe the roadmap should include a minor release at which we just fix
style, including indentation?
Patricia
On 11/29/2010 1:55 AM, Tom Hobbs wrote:
Serious comment:
Shouldn't reset also be synchronized? Also, +1 for making _count volatile.
Frivolous comment:
Can we drop the underscore prefix for member variables? It's against
the official Sun/Oracle/Whatever conventions. Also, it drives me up
the wall...
http://www.oracle.com/technetwork/java/codeconventions-135099.html#367
On Sun, Nov 28, 2010 at 11:16 PM, Patricia Shanahan<p...@acm.org> wrote:
In the way in which this is used, I expect most of the calls to be to
increment. It has to be synchronized, so it seems simplest to synchronize
all the methods.
I've done some more experiments, and decided this is a real problem. As part
of my debug effort, I increased the number of threads in the stress test, so
that it fails much more often. I also added some debug printouts, one of
which was showing up in conjunction with some but not all failures, so I
thought it was irrelevant.
With the additional synchronization, the debug message shows up in all
failures. I think I actually had two forms of failure, one of which is fixed
by the synchronization. In the failure case that has been fixed, everything
works, no debug messages, but the test never admits to having finished,
exactly the symptom I would expect from this issue.
I plan to check in the enhanced test as well as the fixes, because it only
takes a few minutes longer than the current size, and is much better at
finding bugs.
Patricia
On 11/28/2010 2:52 PM, Peter Firmstone wrote:
Well, at the absolute minimum the variable should be volatile, so any
changes are visible among all threads.
Since increment is the only mutating method, this must be synchronized.
This is a cheap form of multi read, single write threading, although one
must be careful, as this only works on primitives and immutable object
references, since only the reference itself is being updated.
If it was a reference to a mutable object (or long), then all methods
would need to be synchronized.
Cheers,
Peter.
Patricia Shanahan wrote:
I've found something I think is a problem in
com.sun.jini.test.impl.outrigger.matching.StressTest, but it does not
seem to be the problem, or at least not the only problem, causing the
test hang I'm investigating. It's difficult to test, so I'd like a
review of my reasoning. This is a question for those who are familiar
with the Java memory model.
Incidentally, if we went to 1.5 as the oldest supported release, this
could be replaced by an AtomicInteger.
In the following inner class, I think the methods reset and getCount
should be synchronized. As the comments note, the operations they
perform are atomic. My concern is the lack of a happens-before
relationship between those two methods and the increment method. As
far as I can tell, there is nothing forcing the change in the counter
due to an increment to become visible to a getCount call in another
thread.
private class Counter {
/**
* Internal counter variable.
*/
private int _count = 0;
/**
* Constructor. Declared to enforce protection level.
*/
Counter() {
// Do nothing.
}
/**
* Resets internal counter to zero.
*/
void reset() {
// Integer assignment is atomic.
_count = 0;
}
/**
* Increments internal counter by one.
*/
synchronized void increment() {
++_count;
}
/**
* Returns current value of this<code>Counter</code> object.
*/
int getCount() {
// Returning an integer is atomic.
return _count;
}
}
