Yes, you're right. I knew about the non-atomicity of ++, my concern was a call to reset creeping in between the two parts of that operation.
I forgot about the "Get a global lock on the variable" that volatile would require as mentioned here; http://www.javaperformancetuning.com/news/qotm051.shtml On Mon, Nov 29, 2010 at 10:50 AM, Peter Firmstone <[email protected]> wrote: > Tom Hobbs wrote: >> >> Serious comment: >> >> Shouldn't reset also be synchronized? > > You've identified that reset is also a mutator method, I guess I gave > mutation as my reason and I wasn't being entirely accurate, sorry. > > To explain a bit more, reset doesn't have to be synchronized when _count is > volatile and increment synchronized, the reason, it's a single operation > mutator, it just resets the counter, it doesn't check it first. > > The increment method is at least two operations _count++ first gets _count, > then increments it and sets _count to the new value, if it wasn't > synchronized, another increment could get _count at the same time... Or a > reset operation could occur in between and the reset fail. > > The ++ operator isn't atomic. > > If in doubt use synchronization, so by making reset also synchronized you > wouldn't be wrong and it probably wouldn't hurt performance either. > > The secret to concurrency is, all operations must happen atomically. > Synchronization creates atomicity when it wouldn't otherwise exist. > Synchronization also effects visibility, as does volatile. > > Cheers, > > Peter. > >> 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; >>>>> } >>>>> } >>>>> >>>>> >>>> >>>> >>> >>> >> >> > >
