Using volatile instead of synchronized for a simulation

Question

I want to know if using volatile in this scenario will give better performance than using synchronized, specifically for the paused and running instance variable in the SimulationManager class.

public class SimulationManager {
 private List<SimulationPanel> simulations;
 private boolean paused = false;
 private boolean running = false;
 private volatile PausableStopabbleThread observer;
 public SimulationManager() {
 simulations = new ArrayList<>();
 }
 public SimulationManager(List<SimulationPanel> simulations) {
 this.simulations = Collections.unmodifiableList(simulations);
 }
 public void start() {
 if (isRunning()) {
 return;
 }
 for (SimulationPanel sim : simulations) {
 sim.tableInsertion.start();
 }
 setRunning(true);
 observer = new SimulationsObserver();
 observer.start();
 }
 public void play() {
 if (!isRunning() && !isPaused()) {
 return;
 }
 setPaused(false);
 for (SimulationPanel sim : simulations) {
 sim.tableInsertion.play();
 }
 observer.play();
 }
 public void reset() {
 if (!isRunning()) {
 return;
 }
 setRunning(false);
 setPaused(false);
 observer.stopWork();
 observer = null;
 for (SimulationPanel sim : simulations) {
 sim.tableInsertion.stopWork();
 }
 }
 public void pause() {
 if (!isRunning() && isPaused()) {
 return;
 }
 setPaused(true);
 observer.pause();
 for (SimulationPanel sim : simulations) {
 sim.tableInsertion.pause();
 }
 }
 private synchronized void setPaused(boolean b) {
 this.paused = b;
 }
 private synchronized boolean isPaused() {
 return paused;
 }
 public synchronized boolean isRunning() {
 return running;
 }
 private synchronized void setRunning(boolean running) {
 this.running = running;
 }
 /**
 * Specifies when the simulations has finished
 * 
 * @author Victor J.
 * 
 */
 class SimulationsObserver extends PausableStopabbleThread {
 private final int nSimulations = simulations.size();
 @Override
 public void run() {
 int finished = 0;
 while (!stopRequested()) {
 for (SimulationPanel sim : simulations) {
 if (!sim.tableInsertion.isAlive()) {
 finished++;
 }
 }
 if (finished == nSimulations) {
 setRunning(false);
 return;
 } else {
 finished = 0;
 }
 pausePoint();
 }
 }
 }
}

Answer 1

volatile should perform better and is appropriate in this situation. A volatile read/write is almost as fast as a non volatile read/write on modern architectures. On the other hand, locking with synchronized will have an overhead, especially in a highly contented scenario (many threads fighting for the lock).

Note however that unless you call the pause/run methods thousands or millions of times per second, you will probably not notice the difference.

Additional comments

This line:

this.simulations = Collections.unmodifiableList(simulations);

probably does not do what you think it does: this.simulations is unmodifiable, but the calling code can still modify the original collection, and the changes will be reflected to your local version (or they won't depending on memory consistency). It would probably be better to do a defensive copy:

this.simulations = new ArrayList<> (simulations);

Another note: because your simulations is effectively immutable,

this.simulations = new ArrayList<> ();

could be replaced by:

this.simulations = Collections.emptyList();

And by the way, you could make simulations final.

• tb-: A volatile read/write is almost as fast as a non volatile read/write on modern architectures I would not agree with this. Even on modern architectures, memory access is a lot slower than level1 or level2 cache access, could be a factor of 50 to 1000. Nevertheless, it does not influence most of the situations, it happens very rarely that this is the limiting factor.
• vjames: Thank you for your additional comments. Making a defensive copy will certainly behave as I want it. Additionally I could: this.simulations = Collections.unmodifiableList(new ArrayList<>(simulations);
• assylias: @tb- On modern architectures, the CPU is generally smart enough to determine when the volatile can be read from L1 only - if the volatile needs to come from the main memory, then yes, you get a performance hit. See for example: stackoverflow.com/questions/4633866/is-volatile-expensive
• assylias: In other words I should have said: uncontended volatile read/write operations are almost as fast as...
• tb-: I did not further investigate it at asm level, but a small test confirmed my numbers. Just tried to read a static int with and without volatile in 1 or multiple threads inside a loop. In all cases significant slower. Did you try it, too?

Answer 2

I do not think, that this will work as you want. In general, I would avoid the use of volatile. Only use it if you really have to, because of some requierements. Otherwise, use the java.util.concurrent.atomic package (they may use volatile, but you do not have to care about the implementation details), use synchronized or other concepts from here: http://docs.oracle.com/javase/tutorial/essential/concurrency/index.html

In your code, this is one of the examples, which could fail:

public void reset() {
 if (!isRunning()) {
 return;
 }
 setRunning(false);
 setPaused(false);
 observer.stopWork();
 observer = null;
 for (SimulationPanel sim : simulations) {
 sim.tableInsertion.stopWork();
 }
}

You can enter the reset method with 2 threads, both are behind the if check. First thread sets observer to null, second throws a NullPointerException. Same happens for other methods.

The easiest way to fix this is to use a mutex-object for nearly all methods. Better solutions depend on further analysis.

---

private synchronized void setPaused(boolean b) { ... }
private synchronized boolean isPaused() { ... }
public synchronized boolean isRunning() { ... }
private synchronized void setRunning(boolean running) { ... }

I would rather use an AtomicBoolean instead of synchronized methods to set a boolean. I would avoid boolean parameters. Use method names like enableRunning(), disableRunning() or something similar. Or setRunning(), unsetRunning(). Or runningEnable(), runningDisable().

---

Some words about volatile. There are (at least?) two signs that volatile is not enough: If a write depends on the current value (Such as var = var + x). Or if the volatile variable depends on some other variable(s) (Such as if(volatileVariable && otherVariable), which is similar to your code). The first can be solved with atomic manipulate functions for primitives, the second needs most probably some sort of mutex or semaphore.

• vjames: Thank you it definitely makes sense to use a mutex object for the unsynchronized methods. Even though in the project scope there is only one thread using the SimulationManager class. But with your answer it made me realized that its better to a have a thread-safe class.