Box2D Forums

It is currently Sat May 18, 2013 9:11 pm

All times are UTC - 8 hours [ DST ]




Post new topic Reply to topic  [ 16 posts ]  Go to page 1, 2  Next
Author Message
PostPosted: Sun Jul 19, 2009 7:19 pm 
Offline

Joined: Sun Sep 23, 2007 2:35 pm
Posts: 803
Just an update on this: it looks like the following files (at least) have non threadsafe operations going on:

Distance.java
TOI.java
CircleContact.java
EdgeAndCircleContact.java
CollideCircle.java
CollidePoly.java
PointAndCircleContact.java
PointAndPolyContact.java
PolyAndCircleContact.java
PolyAndEdgeContact.java
PolyContact.java

So at the moment, JBox2d is not safe to use with multiple worlds simulated in multiple threads, at least without either editing or locking so that the world steps happen sequentially (which would remove any multi-processor benefit that you might otherwise exploit).

The fix here is not entirely straightforward; I'm going back and forth between performing per-world pooling or per-thread pooling. Per-world might work fine for now, but if we ever get around to adding true multi-threading to the engine (i.e. one world, multiple threads), that will mean going back and fixing up all that stuff then, so it might be better just to deal with it now.

I'll post an update when I decide how to best approach this and get some code in.


Top
 Profile  
 
PostPosted: Sun Jul 19, 2009 7:50 pm 
Offline

Joined: Mon Jun 08, 2009 12:21 pm
Posts: 353
I made a lot of the pooling variables static, so a solution for some of these would to just make them member variables. That doesn't work for static methods though. For those, perhaps JStackAlloc is the best bet.


Top
 Profile  
 
PostPosted: Mon Jul 20, 2009 1:32 am 
Offline

Joined: Sun Sep 23, 2007 2:35 pm
Posts: 803
toucansam wrote:
I made a lot of the pooling variables static, so a solution for some of these would to just make them member variables. That doesn't work for static methods though. For those, perhaps JStackAlloc is the best bet.

The main problem with simply changing static pooled Vec2s to members is that for things like contacts (and most of this stuff applies primarily to the contacts), there might be hundreds or thousands in existence, being destroyed and recreated all over the place, and that's going to cause a lot of object creation/GC, barely any savings over the totally unpooled approach if each method is only called a few times. We really don't need a separate copy of each pooled variable for each contact, we just need one for each world/thread.

At the moment, I'm considering managing a simple object pool from the World class, which is easily accessible from pretty much any of the instantiated classes we're having issues with; nothing fancy, we'd just pull in refs to the (hard coded) pooled objects from the world once at each object instantiation, and then use them from there. If at some point in the future we decide we need thread-local pools, that would just mean we need to change a few lines of code to grab a pool based on the thread rather than the world. At the moment we can avoid that, though, since it might be a bit of a performance hit.

For something like Distance, Collide*, or TOI, which have only static methods, I might just turn them into instantiated classes, and make the pooled objects members instead of statics. Each of these would only need to be created once for each world, so that shouldn't be a big deal at all.

I think this would strike a decent compromise between performance and maintainability, while still allowing multiple worlds to run at once. It will definitely require some testing, so I'll try to put together a demo that shows something like this in action, too.


Top
 Profile  
 
PostPosted: Mon Jul 20, 2009 4:02 am 
Offline

Joined: Mon Apr 28, 2008 3:07 am
Posts: 52
your approach sounds very reasonable to me.
thanks for picking up the topic and let me know when I can help you stress-test the multi-world-version.


Top
 Profile  
 
PostPosted: Mon Jul 20, 2009 5:57 am 
Offline

Joined: Mon Jun 08, 2009 12:21 pm
Posts: 353
ewjordan wrote:
toucansam wrote:
I made a lot of the pooling variables static, so a solution for some of these would to just make them member variables. That doesn't work for static methods though. For those, perhaps JStackAlloc is the best bet.

The main problem with simply changing static pooled Vec2s to members is that for things like contacts (and most of this stuff applies primarily to the contacts), there might be hundreds or thousands in existence, being destroyed and recreated all over the place, and that's going to cause a lot of object creation/GC, barely any savings over the totally unpooled approach if each method is only called a few times. We really don't need a separate copy of each pooled variable for each contact, we just need one for each world/thread.

At the moment, I'm considering managing a simple object pool from the World class, which is easily accessible from pretty much any of the instantiated classes we're having issues with; nothing fancy, we'd just pull in refs to the (hard coded) pooled objects from the world once at each object instantiation, and then use them from there. If at some point in the future we decide we need thread-local pools, that would just mean we need to change a few lines of code to grab a pool based on the thread rather than the world. At the moment we can avoid that, though, since it might be a bit of a performance hit.

For something like Distance, Collide*, or TOI, which have only static methods, I might just turn them into instantiated classes, and make the pooled objects members instead of statics. Each of these would only need to be created once for each world, so that shouldn't be a big deal at all.

I think this would strike a decent compromise between performance and maintainability, while still allowing multiple worlds to run at once. It will definitely require some testing, so I'll try to put together a demo that shows something like this in action, too.


I was thinking the exact same thing. Make the static classes member classes with with something like this in world:

Code:
private final CircleCollide circleCollide = new CircleCollide();

public final CircleCollide getCircleCollide(){
   return circleCollide;
}


In terms of changing from static to member variables for actively used classes like contactpoint, yes that would slow things down. That would just be a quick fix. The pool in world sounds good, although if we do end up with multiple threads, we can change the pool management to a static reference, with one pool per thread.


Top
 Profile  
 
PostPosted: Tue Jul 21, 2009 11:18 am 
Offline

Joined: Mon Jun 08, 2009 12:21 pm
Posts: 353
Here's my idea for the object pools (This will be completely thread save now, actually). There are two options.

First option:
Code:
public final class ObjectPool {
   private static final HashMap<Thread, Stack<Vec2>> vec2Pool = new HashMap<Thread, Stack<Vec2>>();
   private static final HashMap<Thread, Stack<Mat22>> mat22Pool = new HashMap<Thread, Stack<Mat22>>();
   // others, etc
   
   private ObjectPool() {
   };

   /**
    * Initializes the pools for the thread. This would normally be in the
    * get____() logic, but to speed things up we just have each thread call
    * this before the pools work.
    */
   public static final void initPools() {
      vec2Pool.put(Thread.currentThread(), new Stack<Vec2>());
      mat22Pool.put(Thread.currentThread(), new Stack<Mat22>());
      // others, etc
   }

   public static final Vec2 getVec2() {
      Stack<Vec2> pool = vec2Pool.get(Thread.currentThread());
      assert (pool != null) : "Pool was null, make sure you call initPools() for each thread";

      if (pool.isEmpty()) {
         pool.push(new Vec2());
         pool.push(new Vec2());
         pool.push(new Vec2());
         pool.push(new Vec2());
         pool.push(new Vec2());
      }

      return pool.pop();
   }

   public static final Vec2 getVec2(Vec2 toCopy) {
      Stack<Vec2> pool = vec2Pool.get(Thread.currentThread());
      assert (pool != null) : "Pool was null, make sure you call initPools() for each thread";

      if (pool.isEmpty()) {
         pool.push(new Vec2());
         pool.push(new Vec2());
         pool.push(new Vec2());
         pool.push(new Vec2());
         pool.push(new Vec2());
      }

      return pool.pop().set(toCopy);
   }

   public static final void returnVec2(Vec2 argToRecycle) {
      Stack<Vec2> pool = vec2Pool.get(Thread.currentThread());
      assert (pool != null) : "Pool was null, make sure you call initPools() for each thread";
      pool.push(argToRecycle);
   }

   public static final Mat22 getMat22() {
      Stack<Mat22> pool = mat22Pool.get(Thread.currentThread());
      assert (pool != null) : "Pool was null, make sure you call initPools() for each thread";

      if (pool.isEmpty()) {
         pool.push(new Mat22());
         pool.push(new Mat22());
         pool.push(new Mat22());
         pool.push(new Mat22());
         pool.push(new Mat22());
      }

      return pool.pop();
   }

   public static final Mat22 getMat22(Mat22 toCopy) {
      Stack<Mat22> pool = mat22Pool.get(Thread.currentThread());
      assert (pool != null) : "Pool was null, make sure you call initPools() for each thread";

      if (pool.isEmpty()) {
         pool.push(new Mat22());
         pool.push(new Mat22());
         pool.push(new Mat22());
         pool.push(new Mat22());
         pool.push(new Mat22());
      }

      return pool.pop().set(toCopy);
   }

   public static final void returnMat22(Mat22 argToRecycle) {
      Stack<Mat22> pool = mat22Pool.get(Thread.currentThread());
      assert (pool != null) : "Pool was null, make sure you call initPools() for each thread";
      pool.push(argToRecycle);
   }
   
   // others, etc
}



Second options (a little slower, but more dynamic):
Code:
public final class ObjectPool {
   private static final HashMap<Thread, HashMap<Class<?>,Stack<?>> objectPool = new HashMap<Thread, Stack<Vec2>>();
   
   private ObjectPool() {
   };

   /**
    * Initializes the pools for the thread. This would normally be in the
    * get____() logic, but to speed things up we just have each thread call
    * this before the pools work.
    */
   public static final void initPools() {
      HashMap<Class<?>, Stack<?>> objectMap = new HashMap<Class<?>,Stack<?>>();
      objectMap.put(Vec2.class, new Stack<Vec2>());
      objectMap.put(Mat22.class, new Stack<Mat22>());
      // others, etc
      objectPool.put(Thead.currentThread(), objectMap );
   }
   
   public static final Object getObject(Class<?> argClass){
      // basically
      HashMap<Class<?>,Stack<?>> map = objectPool.get(Thread.currentThread());
      Stack<?> stack = map.get(argClass);
      
      // something in here for creating more if there are none

      return stack.pop();
   }
}


And that initPools could be removed too, and put into the get logic.

What do you think? I'm leaning more towards the first one.


Top
 Profile  
 
PostPosted: Tue Jul 21, 2009 2:15 pm 
Offline

Joined: Mon Jun 08, 2009 12:21 pm
Posts: 353
Here's the pool so far, notice how I included Distance and Collide classes (which are now completely non static).

Code:
   private static final HashMap<Thread, Stack<Vec2>> vec2Pool = new HashMap<Thread, Stack<Vec2>>();
   private static final HashMap<Thread, Stack<Mat22>> mat22Pool = new HashMap<Thread, Stack<Mat22>>();
   private static final HashMap<Thread, Stack<XForm>> xFormPool = new HashMap<Thread, Stack<XForm>>();
   private static final HashMap<Thread, Stack<Sweep>> sweepPool = new HashMap<Thread, Stack<Sweep>>();
   private static final HashMap<Thread, Stack<Manifold>> manifoldPool = new HashMap<Thread, Stack<Manifold>>();
   private static final HashMap<Thread, Stack<ContactPoint>> contactPointPool = new HashMap<Thread, Stack<ContactPoint>>();
   private static final HashMap<Thread, Stack<RaycastResult>> raycastResultPool = new HashMap<Thread, Stack<RaycastResult>>();
   private static final HashMap<Thread, Stack<BoundValues>> boundValuesPool = new HashMap<Thread, Stack<BoundValues>>();
   private static final HashMap<Thread, Distance> distances = new HashMap<Thread, Distance>();
   private static final HashMap<Thread, CollideCircle> collideCircles = new HashMap<Thread, CollideCircle>();
   private static final HashMap<Thread, CollidePoly> collidePolys = new HashMap<Thread, CollidePoly>();


Top
 Profile  
 
PostPosted: Thu Jul 23, 2009 9:19 am 
Offline

Joined: Mon Sep 01, 2008 6:32 am
Posts: 101
I would use ThreadLocals, not HashMaps. That's what ThreadLocal is made for.


Top
 Profile  
 
PostPosted: Thu Jul 23, 2009 9:41 am 
Offline

Joined: Mon Jun 08, 2009 12:21 pm
Posts: 353
Villane wrote:
I would use ThreadLocals, not HashMaps. That's what ThreadLocal is made for.


Wow, I've never heard of these. The HashMaps are suprisingly fast for now, I'll test out using these after I finish pooling everything

Edit: ThreadLocal is basically a hashmap too. But'll I'll try it out.
Edit2: I think the main difference is that ThreadLocal is (obviously) thread-safe, and it doesn't lock. I don't think HashMap is thread-safe.


Top
 Profile  
 
PostPosted: Thu Jul 23, 2009 12:31 pm 
Offline

Joined: Mon Sep 01, 2008 6:32 am
Posts: 101
toucansam wrote:
Edit: ThreadLocal is basically a hashmap too. But'll I'll try it out.
Edit2: I think the main difference is that ThreadLocal is (obviously) thread-safe, and it doesn't lock. I don't think HashMap is thread-safe.


Hmm... don't remember if HashMap is threadsafe or not, but ThreadLocal should be generally easier to use for things like this. Also, the thread-specific objects will get garbage collected automatically when a thread dies. With a regular HashMap, you'd have to clear those values from the map yourself.


Top
 Profile  
 
Display posts from previous:  Sort by  
Post new topic Reply to topic  [ 16 posts ]  Go to page 1, 2  Next

All times are UTC - 8 hours [ DST ]


Who is online

Users browsing this forum: No registered users and 1 guest


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum
You cannot post attachments in this forum

Search for:
Powered by phpBB® Forum Software © phpBB Group