Added object pooling for quaternions and arrays and started to update the
maths classes to use pooled objects for temporary calculations.
I'd like to propose that I am going to mark as deprecated those methods which only exist to allow the caller to pass in 'working objects' such as #multLocal(TransformMatrix,Vector3f) in TransformMatrix. If people agree on this I'll mark them as deprecated and then remove them in, let's say, 2-3 months time.
If everybody is OK with this then I'll go through the rest of the maths classes and do the same.
One thing to note: I've created an ObjectPool class in com.jme.util, there are a couple of reasons for this:
it seems wrong to have com.jme classes depending upon com.jmex classes; and
the existing object pooling mechanism uses synchronized, which will be problematic with many threads.
also, r.e. synchronized vs. thread local, I think that it would be useful to be able to use the same classes on the client and the server (for multi-player games) and the server environment is probably going to be heavily multi-threaded, so the TL based approach is a huge win there (it should be faster on a modern multi-core CPU based client as well).
The patch is available at http://ianp.org/2009/01/object-pool-patch.txt.gz
Also your character encoding for the patch is somewhat messed up, jME's encoding is (unfortunately) Windows-1252, but at some point in generating your patch something appears to have misinterpreted the source code as UTF-8.
I like the direction of this. Reduces garbage and increases speed as thread locals are supposedly very fast.
I suggest that you take a more centralized approach however, Perhaps all temporary variables should be grouped in one thread local? E.g ObjectPool.get().tempVec, ObjectPool.get().floatArray16, etc. Since each thread has it's own temp var set, there's no need to manage who has access to what.
Using lists to manage all of this, it might actually be slower as lists create garbage as well… It's much better to take a light-weight approach on this I think.
I am rather concerned about the performance loss in a couple of methods actually…
public void lookAt(Vector3f direction, Vector3f up ) {
- tmpZaxis.set( direction ).normalizeLocal();
- tmpXaxis.set( up ).crossLocal( direction ).normalizeLocal();
- tmpYaxis.set( direction ).crossLocal( tmpXaxis ).normalizeLocal();
- fromAxes( tmpXaxis, tmpYaxis, tmpZaxis );
+ Vector3f x = Vector3f.POOL.get();
+ Vector3f y = Vector3f.POOL.get();
+ Vector3f z = Vector3f.POOL.get();
+ z.set(direction).normalizeLocal();
+ x.set(up).crossLocal(direction).normalizeLocal();
+ y.set(direction).crossLocal(x).normalizeLocal();
+ fromAxes(x, y, z);
+ Vector3f.POOL.recycle(x);
+ Vector3f.POOL.recycle(y);
+ Vector3f.POOL.recycle(z);
}
This method alone has 3 list lookups, and 3 'returns to the pool'; where before it used 3 global temp variables w/ no overhead to access at all. This method is used ALOT throughout the jME system and could create a noticeable performance drop (IMO)...
I think in some cases using your pool is very appropriate, however in others global variables might be the way to go...
If I can still offer my two cents here… Part of the rewrite for Ardor3D was to get rid of static variables such as the mentioned helper variables. The reason was multithreaded access and the same static variable being easily munged by two competing threads. So if you think you might want MT access, you may have to make some choices about performance (usually a small hit) vs. function.
If I can still offer my two cents here... Part of the rewrite for Ardor3D was to get rid of static variables such as the mentioned helper variables. The reason was multithreaded access and the same static variable being easily munged by two competing threads. So if you think you might want MT access, you may have to make some choices about performance (usually a small hit) vs. function.
Yes, using static variables the way they are now would cause threading issues. That's why it is preferable to call certain non-GL methods in the GL thread. However the performance hit is not necessary, with the use of thread locals, each thread has it's own private temp variables which means you receive both improved memory usage, performance, and thread safety.
Well, maybe. As long as you don't end up chaining together two methods that make use of the same thread local temp variable. That was always an interesting issue… which temp variable is already being used? Is it tempB that's currently open? etc. etc.
@Momonko_Fan: agreed that having a single thread local would be less overhead then 1 per class, I'll post an updated patch over the weekend.
@nymon: yes, that's right. Although my main concern is not really to improve performance as much as it is to reduce the number of GC pauses. IMHO it's worth sacrificing ~1FPS or so if by doing so it's possible to reduce the number of GC pauses. If the performance actually improves then that's sweet as well
I'll put together some performance tests to try to measure the impact of this change and post the results.
@hevee: oops! I'll make sure the encoding gets set to CP-1252 before it hits the repository.
Hmmm, as I'm using a Mac it seems like I'll have to use ISO-8859-1, as I understand it this is pretty compatible with CP-1252 so I hope there won't be any issues.
Using lists to manage all of this, it might actually be slower as lists create garbage as well. It's much better to take a light-weight approach on this I think.
I don't think that the lists will be a problem really, if you look at the ObjectPool class it has a (user configurable via a system property) limit on the number of pooled instances that will be held, this is low by default ( 8 ), I've also edited the patch to set the initial size of the list to this value, so that should ensure that it never needs to grow and reallocate storage space.
You should avoid having "Vector3f.POOL.recycle(x)". Serving objects from a pool is ok, but when you have to manage the deletion too, it gets unmanageable. I suggest to have a single "Vector3f.POOL.clear()" which just resets the pool to serve the first object again. The point is: its not the memory you want preserve by reusing objects, you just want to avoid new object creation.
@vear: doesn't that make it difficult if multiple methods want to grab objects from the pool? having a single clear method would mean that you'd need to manually keep track of which methods are on the stack and when you need to clear the pool. Being able to return individual objects actually seems like it would be easier.
I think that for most temporary objects they're acquired and recycled in the same method, so it's fairly easy to manage (as long as the methods don't get too big, but that's a code smell anyway).
I used an object pool when picking objects in the scene. I clear the pool before picking, so objects are reused for each pick. The system could be extended to have multiple scopes/multiple pools, so it can be made clear, which pool is used for which high-level operation.
It could happen that the reusable object needs to be returned from the method as a result for evaluation in the higher-level functionality. The high-level functionality initializes the pool, and the low-level methods draw from the pool. When the high-level function finishes, it clears the pool in a single operation.
Micro-managing the pool is wasting CPU time, since those low-level math functions are the ones called most frequently, and every small overhead adds up.
If the number of used temp objects in a method is constant, why not put those into a single ThreadLocal?
The temps could be called as "tmp<classname><methodname><variablename>" for better identification then i did, so there would be no confusion, where each one belongs.
I would suggest that regardless of the merit of such a change, if there is a genuine desire to do a final, stable JME 2 release and draw a line under it then no changes should be made to such core classes unless absolutely necessary.
I would suggest that regardless of the merit of such a change, if there is a genuine desire to do a final, stable JME 2 release and draw a line under it then no changes should be made to such core classes unless absolutely necessary.
Yes! I think it would be wise to hold this one so that we can confidently release 2.0. Of course there is no real timetable laid out, I would like to see the release within the next week or so if we can get a consensus on it.
@vear: OK, I've had a look at the VL engine code and I see what you mean now, your approach is similar to that taken by the Javolution libraries (for real-time Java).
I've also run some performance tests and discovered that the thread local hash look-up is worth bearing in mind. This new version of the patch uses a single context class as vear suggests, but retains the list-of-objects based approach that I used originally (the amount of per thread objects created in the VL engine code seems unnecessarily high).
I'll put together some notes about the numbers that I got and post them here.
I would suggest that regardless of the merit of such a change, if there is a genuine desire to do a final, stable JME 2 release and draw a line under it then no changes should be made to such core classes unless absolutely necessary.
This sounds fair enough. I don't think that the changes are particularly intrusive but if the plan is to get a stable 2.0 out the door ASAP then I agree that we should limit changes to bug-fixes and and API breaks that we want to get in for 2.0 (e.g. int to enum conversion).
