Performance Improvement in PhysicsSpace.java

Hey guys,

Found there were some inefficiencies in data structures in PhysicsSpace.java (e.g. temporary iterators being created unnecessarily). I’ve fixed it and tested the resulting code in a profiler with much improvement. Here is the patch diff & I’d appreciate it if this could be incorporated into the main git/svn repositories:

[java]

This patch file was generated by NetBeans IDE

It uses platform neutral UTF-8 encoding and \n newlines.

— Base (BASE)
+++ Locally Modified (Based On LOCAL)
@@ -46,11 +46,13 @@
import com.jme3.math.Vector3f;
import com.jme3.scene.Node;
import com.jme3.scene.Spatial;
+import java.util.ArrayList;
import java.util.Collection;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
+import java.util.Stack;
import java.util.concurrent.Callable;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentLinkedQueue;
@@ -91,8 +93,8 @@
private Map<Long, PhysicsRigidBody> physicsBodies = new ConcurrentHashMap<Long, PhysicsRigidBody>();
private Map<Long, PhysicsJoint> physicsJoints = new ConcurrentHashMap<Long, PhysicsJoint>();
private Map<Long, PhysicsVehicle> physicsVehicles = new ConcurrentHashMap<Long, PhysicsVehicle>();

  • private List<PhysicsCollisionListener> collisionListeners = new LinkedList<PhysicsCollisionListener>();
  • private List<PhysicsCollisionEvent> collisionEvents = new LinkedList<PhysicsCollisionEvent>();
  • private ArrayList<PhysicsCollisionListener> collisionListeners = new ArrayList<PhysicsCollisionListener>();
  • private Stack<PhysicsCollisionEvent> collisionEvents = new Stack<PhysicsCollisionEvent>();
    private Map<Integer, PhysicsCollisionGroupListener> collisionGroupListeners = new ConcurrentHashMap<Integer, PhysicsCollisionGroupListener>();
    private ConcurrentLinkedQueue<PhysicsTickListener> tickListeners = new ConcurrentLinkedQueue<PhysicsTickListener>();
    private PhysicsCollisionEventFactory eventFactory = new PhysicsCollisionEventFactory();
    @@ -331,7 +333,7 @@
    // }
    private void addCollisionEvent_native(PhysicsCollisionObject node, PhysicsCollisionObject node1, long manifoldPointObjectId) {
    // System.out.println(“addCollisionEvent:”+node.getObjectId()+" "+ node1.getObjectId());
  •    collisionEvents.add(eventFactory.getEvent(PhysicsCollisionEvent.TYPE_PROCESSED, node, node1, manifoldPointObjectId));
    
  •    collisionEvents.push(eventFactory.getEvent(PhysicsCollisionEvent.TYPE_PROCESSED, node, node1, manifoldPointObjectId));
    

    }

    /**
    @@ -361,17 +363,17 @@

    public void distributeEvents() {
    //add collision callbacks
    -// synchronized (collisionEvents) {

  •    for (Iterator&lt;PhysicsCollisionEvent&gt; it = collisionEvents.iterator(); it.hasNext();) {
    
  •        PhysicsCollisionEvent physicsCollisionEvent = it.next();
    
  •        for (PhysicsCollisionListener listener : collisionListeners) {
    
  •            listener.collision(physicsCollisionEvent);
    
  •    //for (Iterator&lt;PhysicsCollisionEvent&gt; it = collisionEvents.iterator(); it.hasNext();) {
    
  •    int listenerSize = collisionListeners.size();
    
  •    while( collisionEvents.empty() == false ) {
    
  •        //PhysicsCollisionEvent physicsCollisionEvent = it.next();
    
  •        PhysicsCollisionEvent physicsCollisionEvent = collisionEvents.pop();
    
  •        for (int i=0;i&lt;listenerSize;i++) {
    
  •            collisionListeners.get(i).collision(physicsCollisionEvent);
           }
           //recycle events
           eventFactory.recycle(physicsCollisionEvent);
    
  •        it.remove();
       }
    

-// }
}

 public static &lt;V&gt; Future&lt;V&gt; enqueueOnThisThread(Callable&lt;V&gt; callable) {

[/java]

Thanks,

  • Phr00t

Do you have a number on how much “much” is? =)

Sure.

ListItr was taking up the most memory in my game so far, all from distributeEvents. After making the changes, I don’t even see ListItr taking up memory anymore on the profiler. A stack is much better use anyway than a linked list for this type of usage.

Keep in mind, this is only in about a 30 second test… with this fix, memory use won’t keep building up in this function.

If you replace the listener list with a SafeArrayList and iterate over the array of listeners instead of get(), get(), get(), it will also be faster and can use the nicer loop syntax.

@pspeed said: If you replace the listener list with a SafeArrayList and iterate over the array of listeners instead of get(), get(), get(), it will also be faster and can use the nicer loop syntax.

I don’t have commit privileges on the new git repository, but your suggestion sounds good :slight_smile:

Also it should be done on both jbullet and bullet physics spaces. You don’t need commit privileges in git, thats the advantage of git. You can just make a pull request.

I pulled the git repository, but when trying to push, I get this error:

[java]phr00t@phr00tlaptop:~/jme3_git/jme3-jbullet/src/main/java/com/jme3/bullet$ git push origin phr00tchanges
Username for ‘https://github.com’: phr00t
Password for ‘https://phr00t@github.com’:
remote: Permission to jMonkeyEngine/jmonkeyengine.git denied to phr00t.
fatal: unable to access ‘https://github.com/jMonkeyEngine/jmonkeyengine.git/’: The requested URL returned error: 403
[/java]

I also seem to have lots of other changes to files, looks like version numbers 3.0.0 to 3.0.10 in the “git diff”, and it looks like I can’t just push the PhysicsSpace changes. Not sure I understand this git repository yet but any help will be appreciated…

“push” != “make a pull request”

@pspeed said: "push" != "make a pull request"

Thank you captain obvious. I’d appreciate some help, though. I’m deep in working on my project & I’m trying put a little time in contributing back to jMonkeyEngine.

I already pulled the repository and I’d like to push the PhysicsSpace changes back somewhere so they can be merged.

EDIT: Apparently I have to do something through the website to initiate a pull request, and it isn’t a command line thing. This is very different than the svn system. I’m working on it…

EDIT #2: Don’t have time for this now, but this is more complicated than I was hoping for. Ugh. I may get to it eventually…

You make a fork in your own account, commit only the changes you make in the files, push them to your repo and then send us a pull request. You really made it sound like theres no manual for GitHub so I don’t see a reason to attack Paul here.

1 Like
@normen said: You make a fork in your own account, commit only the changes you make in the files, push them to your repo and then send us a pull request. You really made it sound like theres no manual for GitHub so I don't see a reason to attack Paul here.

I haven’t slept much – newborn in the house. Sorry Paul. However, a response of “a pull request isn’t the same as the pull you already did, look up how to do a pull request” would have been so much more helpful & appreciated. Git has command line “pulls” which I thought were the same as making a “pull request”.

Anyway, thank you for the clarification. I’ll have to get to this at some point…

EDIT: Think I got it figured out & submitted.

@phr00t said: Sure.

http://imgur.com/a/DWsnA

ListItr was taking up the most memory in my game so far, all from distributeEvents. After making the changes, I don’t even see ListItr taking up memory anymore on the profiler. A stack is much better use anyway than a linked list for this type of usage.

Keep in mind, this is only in about a 30 second test… with this fix, memory use won’t keep building up in this function.

Stack is not recommended, read docs.
Try use Deque<T> stack = new ArrayDeque<T>();

1 Like
@kles4enko said: Stack is not recommended, read docs. Try use Deque<T> stack = new ArrayDeque<T>();

Which docs say not to use Stack?

EDIT: Wow, right there in the main Oracle documentation. Odd how when you want a stack, you shouldn’t use Stack. Ha! Thanks for the heads-up.

Actually, I’m not really familiar with this class but wouldn’t you want to handle events in the same order they were added? I would think a queue is more appropriate. But again, I don’t really know what’s going on.

@pspeed said: Actually, I'm not really familiar with this class but wouldn't you want to handle events in the same order they were added? I would think a queue is more appropriate. But again, I don't really know what's going on.

Didn’t think about that… not sure if that matters. Basically, it is making sure all of the collision listeners are getting all of the collision events. I don’t know if there is any particular order collisions come in, but interesting observation anyway.

Deques are only valid from java 6 on btw, jME has Java 5 as a baseline.

About that…It’s a bit off topic but this PR could benefit from the decision.
According to this http://www.oracle.com/technetwork/java/eol-135779.html java 5 support (extended support w/e that means) run until 2015. Since it’s in about a little bit more than half a year couldn’t we state that JME 3.1 baseline is java 6?
Not sure that a lot of people are still using jme with java 5 anyway. What do you think?
This would allow to use Deques in this PR.

1 Like

If you guys decide to use Java 6, I can update the pull request with Deques (once I figure out how to do it, hehe).

In applications I’m using Java 8 :wink: In libraries Java 6 is fine. What version lwjgl supports?

@nehon said: About that...It's a bit off topic but this PR could benefit from the decision. According to this http://www.oracle.com/technetwork/java/eol-135779.html java 5 support (extended support w/e that means) run until 2015. Since it's in about a little bit more than half a year couldn't we state that JME 3.1 baseline is java 6? Not sure that a lot of people are still using jme with java 5 anyway. What do you think? This would allow to use Deques in this PR.

Well i would say 6 is everywhere stable and supported (read all platforms) so I think its time to move on. After all we can’t stay at 1.5 for ever, and if someone is really still using it, they could just fork jme3 and add a small patch for it locally. (I assume if so it would only be corporations, no hobbyists)