HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Using volatile instead of synchronized for a simulation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
simulationvolatileinsteadsynchronizedforusing

Problem

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 simulations;
private boolean paused = false;
private boolean running = false;
private volatile PausableStopabbleThread observer;

public SimulationManager() {
simulations = new ArrayList<>();
}

public SimulationManager(List 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 run

Solution

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.

Code Snippets

this.simulations = Collections.unmodifiableList(simulations);
this.simulations = new ArrayList<> (simulations);
this.simulations = new ArrayList<> ();
this.simulations = Collections.emptyList();

Context

StackExchange Code Review Q#25011, answer score: 2

Revisions (0)

No revisions yet.