In the math package, these methods all use static temporary variables. Could somebody add warnings to the javadocs that they're not threadsafe? I got bitten by lookAt, so I copied the code and used my own temp variables I then hunted for the rest of them in com.jme.math, so this is an exhaustive list. Thanks!
Matrix3f.fromAxisAngle()
Quaternion.lookAt()
Plane.setPlanePoints()
Ray.intersects*()
Ring.random()
Rather than adding warnings, perhaps it would be better if they were fixed to provide thread-safety as these methods could wreak havoc in a game without being obvious where to find the cause.
I'll have a look at these… fixing them MUST be done using statics safely. Creating new objects for helper reasons in math methods is not an option.
Although it would cause performance degradation you could simply solve the problem by making the methods synchronized. Really, that's likely your only solution if you want to continue to utilize the static objects.
However, if you wanted to go for a more advanced concept that could be adapted throughout jME, an "ObjectPool" could be generated for jME that could have methods like getVector3f() that would create a new Vector3f the first time it is used and it could be returned when it's finished…additional Vector3fs are created as necessary if all the currently existing objects are already in use. That would keep performance high, keep efficiency, and solve threading issues.
Yeah right, pooling inside a math method will keep performance high. My guess is even synchronized would be faster.
Another alternative is something like LWJGL's GLContext.getContextCapabilities(), using thread local variables with caching for repeated acces from the same thread. Take a look at that code if you want to do something with that. There's a branch in there, but a predictable one. If it's fast enough… maybe in combination with a run time flag.
Or the almost obvious one, using different FastMath instances instead of static methods.
I will revert any use of synchronized, pooling or unneeded object creation inside FastMath methods, unless it's renamed SlowMath first 
FastMath instances would be pointless as you would have to create an instance of FastMath even for a single method call.
Pooling across jME would be nice in general because we use a lot of these temp variables. Personally I would say that is the way to go.
@sfera: Well, what I mean is replace static methods (edit: hm non static too I guess, yes this would get a little bit too ugly) from different classes, with FastMath methods. Where there are different FastMath objects for different tasks/threads. (these can be put in static fields, for example). Ok, not THAT obvious I guess…
@renanse/all: does anyone have any proof that pooling is at all faster than what a VM can do? Pooling itself can be rather expensive, espc. if you want to do this in a thread safe way like what's suggested here. Compare this with the vm creating a short lived object for that specific thread if you use new. AFAIK pooling is only useful if you have large objects that have to do a lot of instancing of other object when they are created.
@llama: i still don't understand what you mean. sorry
@renanse: i have to agree with llama about pooling. it might make things thread safe but i'm quite sure FastMath will become SlowMath.
I would be happy to write a proof-of-concept to validate this theory of pooling and performance.
Pooling is potentially significantly better than synchronized blocks in situations where you could have many different threads accessing the same instance and causing every thread to get stuck in a waiting queue. This is likely not going to be too bad since they would be in FastMath, but overrall I think that jME seriously needs to start using pooling since like Renanse said, we've got local instances everywhere and we'd probably see some significant memory use reduction going this route as well.
If you've been involved with the jME project for awhile, you'll recall that a few years back our memory use was tremendous and temporary object creation throughout the code was causing major hiccups during run time. I don't know how many hours I spent working on that and the result is we have a footprint that is significantly lower than other java engines and the old gc hiccup plague is gone (unless you have object creation issues in your own code.)
Being an oldtimer and remembering the past here… I just really don't want to go back on these major improvements for the sake of engine enforced thread safety, so I guess I'm just saying let's be nice and careful about any such tweaks. 
Well, there should be no concerns over hiccups if we moved toward object pooling. However, I am not positive that using an object pool wouldn't reduce performance.
Here's my extremely quickly written ObjectPool:
import java.util.concurrent.*;
/**
* ObjectPool provides a convenient pooling of a specific type of objects that can
* be dynamically expanded to provide a reusable system for objects to be gotten
* and then returned so they can be reused. This class should be entirely thread
* safe and provide extremely good performance.
*
* @author Matthew D. Hicks
*
* @param <T>
*/
public class ObjectPool<T> {
private ConcurrentLinkedQueue<T> available;
private Class<T> c;
private volatile int total;
public ObjectPool(Class<T> c) throws InstantiationException, IllegalAccessException {
this(1, c);
}
public ObjectPool(int initialSize, Class<T> c) throws InstantiationException, IllegalAccessException {
available = new ConcurrentLinkedQueue<T>();
this.c = c;
for (int i = 0; i < initialSize; i++) {
available.add(createInstance());
}
}
private T createInstance() throws InstantiationException, IllegalAccessException {
total++;
return c.newInstance();
}
/**
* Retrieve a object of type <code>T</code> from the pool. If no objects are currently
* available a new one will be instantiated and given.
*
* @return
* T
*/
public T get() {
if (available.size() == 0) {
try {
return createInstance();
} catch(Exception exc) {}
}
return available.poll();
}
/**
* When an object has been finished being utilized it should be returned back to the pool
* for reuse via this method.
*
* @param t
*/
public void put(T t) {
available.add(t);
}
/**
* Retrieves the number of available objects in the pool.
*
* @return
* int
*/
public int available() {
return available.size();
}
/**
* Retrieves the size of the queue if all created objects were returned.
*
* @return
* int
*/
public int size() {
return total;
}
}
There is quite a bit I would add before it should be committed but this gives a good example of what I invision. I think the performance hit on this should be extremely minimal, but we need to do some benchmarks to validate that. Constructive criticism is always welcome. ;)
(while darkfrog was posting…)
Pooling by thread would make some sense.
Having a Map based on Thread id to keep temp variables specific to each thread would actually be a very fast lookup and fix the threading issue.
The lookup itself would not have to be synchronized unless there is a new set of temps being added to the map, that can be handled by creating an inital pool with say 10-20 (configurable) sets of temp vars each associated with the first 10-20 thread ids (since thread ids tend to be sequential starting at 0).
For a little bit of overhead in memory I don't see how it would take that much extra cpu time since Map lookups are some of the fastest.
And, if you are really worried about performance, allocate an array.
All, right (while writing this post)… I've put together a little example app:
Results:
# = 100 iterations = 100 = 10000 samples
____________________________________________________
@sfera: nevermind, that one wasn’t a good idea. The getContextCapabilities() approach is worth looking into though.
@darkfrog: It’s not even the accesing the same method (and then blocking) that I’m worried about. Just obtaining a lock for synchronized is damn expensive. It’ll also prevent the VM from using thread specific variables (without having to synch them anyway), which will make the mathmatics themselves slower).
So, pooling then? Aside from the fact that pooling itself will have a lot of overhead, you’ll still have to make it thread safe in this case. So that means either thread local pools (A la getContextCapabilities()) or or synchronization. And this “penalty” will have to paid twice instead of once (taking from the pool and giving back to it). A pool simply makes no sense to me if you have a known number of fields that you want to use (as is the case here, with these math related methods).
Ok, while I was eating I see you made an example. You use ConcurrentLinkedQueue…! Concurrent Linked Queue… Performence
This is what I’ve been talking about and telling you to look at:
public static ContextCapabilities getCapabilities() {
CapabilitiesCacheEntry recent_cache_entry = fast_path_cache;
// Check owner of cache entry
if (recent_cache_entry.owner == Thread.currentThread()) {
/* The owner ship test succeeded, so the cache must contain the current ContextCapabilities instance
* assert recent_cache_entry.capabilities == getThreadLocalCapabilities();
*/
return recent_cache_entry.capabilities;
} else // Some other thread has written to the cache since, and we fall back to the slower path
return getThreadLocalCapabilities();
}
Maybe not that easy to see what it does (of course, take a look at the full code in LWJGL), but I'll explain what jME will have to do in pseudo code in my next post.
@guurk, a map with a thread id based entry is called a ThreadLocal, it's exactly what I've been trying to get everyone to look at! It's even faster if you cache the look ups for the map.
darkfrog's solution although elegant is using a ConcurrentLinkedQueue and a volatile variable.
My guess is that it would create much overhead during lookup.
Thus, I've extended the posted app to use darkfrog's object pool. (I can post if you really, really want to see it, but I figure y'all got the idea.)
Times for using ObjectPool area almost exactly as they would be using a synchronized method.
# = 100 iterations = 100 = 10000 samples
____________________________________________________
I still think it's worthwhile to do some performance benchmarks on this because I'm not convinced that class I created is going to have any significant impact on performance…granted I've been wrong before * insert your gasp of disbelief here *, but I still think it's worth a try.
llama said:
@guurk, a map with a thread id based entry is called a ThreadLocal, it's exactly what I've been trying to get everyone to look at! It's even faster if you cache the look ups for the map.
Well, there we go then ;).
okay, so we cross posted.
I'll play around with this tonight hopefully and see if I can't get the performance close to how fast it is now.