I’m pretty much a noob when it comes to threads and I’ve hit a knot. I do use threading but they do their own things, not actively interacting with the others, but in this particular case, it is.
I have a class called Overseer that I use to load/save a game and/or build the world. This thread will toggle a flag called “isLoading” that is used when either loading a game or building a world (obviously).
Now, in my game’s thread simpleUpdate I poll Overseer to check if it’s “loading”. If it is, certain part of the code isn’t run. The problem is that when I call the method and the Overseer is working the renderer waits until it’s done which is useless in my case because the GUI doesn’t get updated with the work that is being done.
The method is as simple as it gets:
[java]
public synchronized boolean isLoading() {
return isLoading;
}
[/java]
What I want is to get the value and run (metaphorically) out of there to continue doing whatever is needed to be done.
I found several solutions, but all involved using Observables/Observers or really complex (-looking) solutions.
Would using Atomic help in this case?
What would be the faster/simplest way to achieve that?
Thanks.
—
EDIT: Bleh. Just me being slightly dumb.
Now using “synchronized (this) {}” when changing values work as it was before…
Nothing to see here. Move along.
There are several solutions this could be done, yes.
Perhaps using an ExecutorService could work well in your case?
An ExecutorService manages threads (in pools usually) and you could pass it a Callable with whatever you want it to perform, on which you receive a Future object.
Then you can regularly check the Future object to see if a result is available and then get that result, or cancel the work in the thread before it’s done, if you need to.
This “task based” way of working with multiple threads is, I find, one of the easiest ways to get it working well.
There is no reason to synchronize on a simple getter anyway. Only when you have coordinated state does it matter. All the synchronized buys you in this case is a forced memory barrier which really only matters in multi-core setups (which is a conversation deeper than we need to go into here).
In this case, you could use an AtomicBoolean and be synch free and multi-core safe.
Alright then, I’ll go that way tomorrow. I’ll let you know if there’s something not working.
What pspeed said.
The best book on the subject must be “Java Concurrency in Practice” by Brian Goetz should you ever feel the need to dig deeper.
Not that I expected trouble, but thing went smoothly.
Thanks.
Edit: @jmaasing I’ll try to put my hands on that book somehow. Thanks a lot.
I feel the need to revisit the issue of threading. It’s on a slightly different topic.
My caching system consists of mainly 2 classes.
LeafManager.java
This class holds the information of the current sector the player is in: “currentLeaf”
Also holds the LeafCache class variable.
LeafCache.java
The LeafCache class contains all the leafs adjacent to the leaf into which the player finds itself in.
This class also contains other pertinent information. Mainly several lists containing visited leafs, alien homeworlds, list of cached leafs (their ids only), etc. finally the cached leafs themselves.
My main question pertains to the synchronization of the data inside LeafCache. (The cache itself isn’t publicly available, thus only LeafManager has access to it, ergo, based on what’s going on, LeafManager will trigger the cache updates.)
Now, the way I do things looks like this:
[java]
/**
- Sets the middle coordinates of the galaxy. Technically should always be Vector3f.ZERO.
-
@param middleOfGalaxy Vector3f representing the middle coordinates
- of the galaxy.
*/
void setMiddle(Vector3f middleOfGalaxy) {
synchronized (leafCache) {
if (leafCache != null) {
leafCache.setMiddleOfGalaxy(middleOfGalaxy);
}
}
}
[/java]
My main concern is that doing it this way might affect performance. Would it be preferable to synchronize inside LeafCache or it wouldn’t make any difference?
Instead I would end up doing something like that:
[java]
/**
- Sets the middle coordinates of the galaxy. Technically should always be Vector3f.ZERO.
-
@param middleOfGalaxy Vector3f representing the middle coordinates
- of the galaxy.
*/
void setMiddle(Vector3f middleOfGalaxy) {
if (leafCache != null) {
leafCache.setMiddleOfGalaxy(middleOfGalaxy);
}
}
[/java]
The LeafCache method would be changed that way:
[java]
synchronized void setMiddleOfGalaxy(Vector3f middleCoordinates) {
this.middleOfGalaxy = middleCoordinates;
}
[/java]
Is there a benefit from using the second version? Would there be a better way?
Depending on the usage of the cache (read/write operations) implementing a read/write lock might be a good way:
http://docs.oracle.com/javase/6/docs/api/java/util/concurrent/locks/ReentrantReadWriteLock.html
Also depending on the internal design of the leafCache only synchronizing the leaf you are working with would be a good idea.
For the specific operation in your example above using:
[java]
void setMiddle(Vector3f middleOfGalaxy) {
if (leafCache != null) {
leafCache.setMiddleOfGalaxy(middleOfGalaxy,clone());
}
}
Vector3f getMiddle() {
if (leafCache != null) {
return leafCache.getMiddleOfGalaxy();
}
}
Vector3f LeafCache.getMiddleOfGalaxy(){
return muddleOfGalaxy.clone();
}
[/java]
Should already be threadsafe. AFAIK no need to sychronize as long as you don’t modify the object (which you don’t by using only clones…)
@madjack said:
Now, the way I do things looks like this:
[java]
/**
* Sets the middle coordinates of the galaxy. Technically should always be Vector3f.ZERO.
* @param middleOfGalaxy Vector3f representing the middle coordinates
* of the galaxy.
*/
void setMiddle(Vector3f middleOfGalaxy) {
synchronized (leafCache) {
if (leafCache != null) {
leafCache.setMiddleOfGalaxy(middleOfGalaxy);
}
}
}
[/java]
My main concern is that doing it this way might affect performance. Would it be preferable to synchronize inside LeafCache or it wouldn't make any difference?
Instead I would end up doing something like that:
The LeafCache method would be changed that way:
[java]
synchronized void setMiddleOfGalaxy(Vector3f middleCoordinates) {
this.middleOfGalaxy = middleCoordinates;
}
[/java]
Is there a benefit from using the second version? Would there be a better way?
synchronized version of LeafCache#setMiddleOfGalaxy would take the mutex on the leafCache instance.
synchronized (leafCache) { ... would take the mutex on the leafCache instance
So no semantic difference that I know of.
I find that the first version (synchronized methods) is more readable and also puts the synchronization keyword in the same file (the class) that you want to protect from concurrent modifications so that is my preference.
But to be really sure make a simple profiling app. It might be the case that there are faster ways to lock the cache in the concurrency package depending on read/write patterns and so on.
I forgot to mention that the whole thing is threaded for one and only one purpose: so disk fetching/saving isn’t done on the render thread as this tanks the performance obviously. So, in a way this isn’t really about data synchronization. Well, not 100%.
The way things work is that leafManager will trigger a state change in leafCache. The other thread (managed by the Overseer) check at each frame if that state has changed. If it has, the Overseer triggers the cache update and those leafs that are stale are saved to disk if there was any change, removed from the cache and finally the new leafs are loaded. That’s where the data has to be accurate, but except for this, no other thread fiddles with the cache.
@zzuegg
Yeah. The retrieved data isn’t modified, so using a getter isn’t a problem. That’s why those methods are not synchronized. The setters are those that I’m questioning.
@jmaasing said:
So no semantic difference that I know of.
I find that the first version (synchronized methods) is more readable and also puts the synchronization keyword in the same file (the class) that you want to protect from concurrent modifications so that is my preference.
I guess my question becomes: Does it make a difference when the lock is made on leafCache from a synchronized block inside LeafManager vs a synchronized method inside LeafCache?
My tentative answer from what I've read tells me no, but I honestly don't know much.
“synchronized” is just as slow either way. Moving it around isn’t going to help or hurt.
Does “setMiddleOfGalaxy” do anything but set a reference?
If not then you can just make the reference volatile or use the already provided AtomicReference class… either of which would avoid “synchronized”.
If it does then there is still a pretty easy way to avoid “synchronized” but I won’t bore you with those details unless they are needed. 
I think @pspeeds fear of the synchronized statement is really low level stuff… I agree that one should avoid using it if its possible but having a synchronized method being called even 4 times per update loop does virtually nothing to the framerate, when I removed the old synchronized calls from jbullet there was zero performance improvement. So I wouldn’t avoid them like hell if its the easiest way to provide safe access to data but in the end its mostly a hackfor not properly designing fork/join or async access 
Actually I bet that the jit does a analysis on the synchronize statements, and trys to not make a full synchronize, but a more selective on transparently.
Anyway better a synchronize than a crash.
Actually I guess you should move most of your logic into the Leafmanager then.
Most brute way would be to have a Explicit lock that prohibits ANY reading while a write is in place.
Depending on the locking algorithm this is really fast.
Alternativly you can just a Queue whit tasks and a callback when task is done. That way you could probably get away completly synchronize and lock less, due to intelligent Queue behaviour.
Also I sue several dozen synchronized in a update, without mesurable influrence on performance. The parralelism on the other hand really helped the performance.
(Short better waste 1% performance if you get a additional core for this still a yield of 99%)
You might be surprised. And in my various microbenchmarks over the years, Hotspot appears to do nothing special with synch blocks.
And anyway, using a synchronized unnecessarily is like going downstairs to use the bathroom when there is a perfectly good one three feet away upstairs. I don’t use the bathroom that often but why bother with the trouble?
There is a reason all of those nice java.util.concurrent classes exist. 90% of the time it is to do something more efficient than synchronized. Like, I think AtomicReference works out to be twice as fast a single synch-controlled field… even faster if there is ever contention between threads. And you don’t have to worry about which object you synched on, whether you are going to also synch on that in some other way later and screw yourself, etc… it does what you want to do.
app.enqueue() would make this so easy!
@bgilb said:
app.enqueue() would make this so easy!
Yeah, async or fork/join is generally the best idea for threads talking to each other or offloading work to threads.
pspeed said:
You might be surprised. And in my various microbenchmarks over the years, Hotspot appears to do nothing special with synch blocks.
And anyway, using a synchronized unnecessarily is like going downstairs to use the bathroom when there is a perfectly good one three feet away upstairs. I don't use the bathroom that often but why bother with the trouble?
There is a reason all of those nice java.util.concurrent classes exist. 90% of the time it is to do something more efficient than synchronized. Like, I think AtomicReference works out to be twice as fast a single synch-controlled field... even faster if there is ever contention between threads. And you don't have to worry about which object you synched on, whether you are going to also synch on that in some other way later and screw yourself, etc... it does what you want to do.
Hotspot does a lot of things with synchronized, it does inlining and escape analysis and removes synchronized if it decides it is not needed (here is one example: http://blog.nirav.name/2007/02/escape-analysis-in-mustang-ii.html). It might even rearrange your code execution to make better use of registers if it preserves the memory model for other threads.
But of course that does not happen magically in every case, I guess the usual case is that synchronized is needed where it is written so you have to pay the price of locking the mutex. This operation certainly takes some time but not anything like the horror stories from 10 years ago.
The performance power (I think) in the AtomicXXX is that they can perform CompareAndSet operations (CAS). This might even be a primitive of the CPU of the native platform and performs pretty good in that respect. If the CAS operations on AtomicXXX isn't used it is usually as efficient as a volatile variable access (which is still pretty good in the java world).
But the real power of the concurrency package in java is that it is written by people that really know what they are doing and you probably can't do better :)
Like pspeed said it is also my experience that the Locks in the concurrency package performs better than synchronized when you have high lock contention (sometimes very much better). But it doesn't sound like that is a problem in this case (madjack probably doesn't have >10 threads reading from the cache at the same time it is updated).
But the most important lesson I've learned is what Knuth said "premature optimization is the root of all evil". So use synchronized and make it thread safe and correct, and then measure if it actually is a problem, and only then look at optimizations suited for your access pattern, which might very well be what normen said, change the design :)
@jmaasing said:
But the most important lesson I've learned is what Knuth said "premature optimization is the root of all evil". So use synchronized and make it thread safe and correct, and then measure if it actually is a problem, and only then look at optimizations suited for your access pattern, which might very well be what normen said, change the design :)
Totally. "Premature optimization" only gives big time headaches during development. And the best thing is when you got a generally working "non-optimized" application you can start mice-optimizing so much better and its so much fun then ;)
@jmaasing said:
Hotspot does a lot of things with synchronized, it does inlining and escape analysis and removes synchronized if it decides it is not needed (here is one example: http://blog.nirav.name/2007/02/escape-analysis-in-mustang-ii.html). It might even rearrange your code execution to make better use of registers if it preserves the memory model for other threads.
But of course that does not happen magically in every case, I guess the usual case is that synchronized is needed where it is written so you have to pay the price of locking the mutex. This operation certainly takes some time but not anything like the horror stories from 10 years ago.
I performed some microbenchmarks about a year ago when I was designing my threaded buffers for my networking interpolation and doing some other unrelated JME optimizations. This included an inner loop synchronized test without any contention (ie: single threaded)... in 4000 runs the timings never changed significantly so Hotspot didn't really do anything for it. The JIT may already have optimized what it could. The loop with synch took about 2-3 times as long as the one without. The inner loop operation was an ArrayList.get(int) call. Under contention this will of course be much much worse.
In a modified version of the tests (done by a fellow engineer) the results seem much worse in the uncontended case. I can provide either tests if someone wants to check the methodology and/or run them themselves.
At any rate, the reason I take pause seeing "synchronized" is because it is so often a sign of code that had no awareness of the java.util.concurrent package... and also likely that threading was never seriously considered in the design. Both potential signs of trouble. It's not that I hate it... I use it too when appropriate but a) it's the sign of bad things coming and b) there is almost always a more elegant design that completely eliminates the need. Especially in a run-loop driven environment like JME.
@pspeed said:
I performed some microbenchmarks about a year ago when I was designing my threaded buffers for my networking interpolation and doing some other unrelated JME optimizations. This included an inner loop synchronized test without any contention (ie: single threaded)... in 4000 runs the timings never changed significantly so Hotspot didn't really do anything for it. The JIT may already have optimized what it could. The loop with synch took about 2-3 times as long as the one without. The inner loop operation was an ArrayList.get(int) call. Under contention this will of course be much much worse.
In a modified version of the tests (done by a fellow engineer) the results seem much worse in the uncontended case. I can provide either tests if someone wants to check the methodology and/or run them themselves.
At any rate, the reason I take pause seeing "synchronized" is because it is so often a sign of code that had no awareness of the java.util.concurrent package... and also likely that threading was never seriously considered in the design. Both potential signs of trouble. It's not that I hate it... I use it too when appropriate but a) it's the sign of bad things coming and b) there is almost always a more elegant design that completely eliminates the need. Especially in a run-loop driven environment like JME.
Just to be clear, I didn't doubt your results, I have seen the same in my benchmarks and as you said; 'you might be surprised'. Just wanted to point out that hotspot might remove the synchronized if it wasn't needed and there are tests to show that it can happen.