> As I said in the original e-mail "I think the methods reset and getCount > should be synchronized.".
Yes, you're right. Sorry, my mistake. I've just finished "Java Concurrency in Practice" which means I can remember just enough to be dangerous, but not quite enough to fool-proof. "volatile" is referred to as a 'poor man synchronisation' and is no means perfect in all cases. Synchronizing the methods makes this less-difficult to make mistakes with, so given that performance isn't an issue I'd got for that rather than synchronizing on _count. Regarding format and layout, I agree that the poor formatting makes reading source files far more difficult than the odd underscore creeping in. My approach is that when I come across such a file, before I do anything else to it, I reformat and commit the file back in with a "reformat" comment. Most diff tools can be convinced to ignore whitespace when comparing different versions. I find the only time changing the layout becomes a problem is when a single commit contains both layout and code changes. That's just my opinion though. On Mon, Nov 29, 2010 at 10:45 AM, Patricia Shanahan <[email protected]> wrote: > 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<[email protected]> 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; >>>>> } >>>>> } >>>>> >>>> >>>> >>> >>> >> > >
