Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dispatcher.GetManifoldByIndexInternal() generates garbage. #22

Closed
Zolniu opened this issue Apr 30, 2015 · 7 comments
Closed

Dispatcher.GetManifoldByIndexInternal() generates garbage. #22

Zolniu opened this issue Apr 30, 2015 · 7 comments

Comments

@Zolniu
Copy link

Zolniu commented Apr 30, 2015

According to Wiki ( https://github.com/AndresTraks/BulletSharp/wiki/Collision-Callbacks-and-Triggers ), the best way to determine collisions is to iterate over all contact manifolds using GetManifoldByIndexInternal method to access each manifold. In Bullet (C++) this requires no memory allocation since all the manifolds are already created and only pointer is returned. However, in BulletSharp the natvie object is wrapped in new PersistentManifold object every time the GetManifoldByIndexInternal method is called. It produces a lot of garbage for GC. Around 300 - 400 collision objects yields one garbage collection every second if all the manifolds are iterated over!

I see two ways to resolve this - either have Dispatcher keep separate cache of PersistentManifold wrappers so it can return existing object when GetManifoldByIndexInternal is called, or change PersistentManifold wrapper to struct so it doesn't allocate memory trough GC when returned.

@AndresTraks
Copy link
Owner

Good point. I'll consider the options.

@Zolniu
Copy link
Author

Zolniu commented Apr 30, 2015

Wow, didn't expect such quick response! :) Currently I'm working on cache implementation in my client code. The only problem I have is to detect when new manifolds are added internally by Dispatcher. Once I have it figured out I'll post my solution - may be it'll help you.

Keep up the good work!

@AndresTraks
Copy link
Owner

Yeah, manifolds are hard to track since there are so many generated and handling them is performance-critical. Detecting when new manifolds are created is hard and it's also hard to detect when they are removed, which is again important for keeping garbage down.

I'm not sure if making PersistentManifold a struct helps, because they will be boxed into objects (e.g. when returned from GetManifoldByIndexInternal), so they'll still need to be garbage-collected.

This hasn't been a big priority so far, so I haven't thought about it very much. If you can figure it out, then that would be great. Thanks!

@AndresTraks
Copy link
Owner

Here's a patch that turns PersistentManifold into a value struct. Can you test it?
http://pastebin.com/5d72sLqS

The public PersistentManifold constructors are removed, but this is OK, because manifolds should only be created in Dispatcher::GetNewManifold anyway.
Also, since structs can't be inherited, PersistentManifold no longer inherits from TypedObject, but that probably isn't strictly necessary for anything.

After 7 years of .NET development, the GC is still confusing as hell :). But if I had to guess, there's less garbage generated with this patch since without the finalizer, the manifolds can be collected in GC generation 0 (if the PersistenManifolds should get boxed into objects).

@Zolniu
Copy link
Author

Zolniu commented May 2, 2015

Ok, I'll test it once I have some time for it. I have forked your repository so you can push your fix in to it:

https://github.com/Zolniu/BulletSharp

In the meantime I've found quite good workaround for this problem. I use gContactProcessedCallback callback to store information about collisions and then I can work with them in my code. It produces a lot less garbage - I can have custom logic for over 800 colliding objects and I'm getting one garbage collection every few minuts. I had to modyfi PersistentManifold::onContactProcessed method to only fire if processed CollisionObject has CustomMaterialCallback flag. You can check it here:

https://github.com/Zolniu/BulletSharp/blob/master/src/PersistentManifold.cpp

About GC, it can be tricky indeed. Not only boxing can lead to "hidden" allocations, foreach over IEnumbrable also allocates memory, but only if used via IEnumerable interface. For example List provides explicit GetEnumerator() method which returns enumerator structure so foreach over List does't generate garbage, but if you change your reference to be IList (interface) it will generate garbage since foreach will use GetEnumerator() via IEnumerable interface inherited by IList interface.

@Zolniu
Copy link
Author

Zolniu commented May 16, 2015

I've made some test in my with my current setup and it is quite obvious that after changing Persistent Manifold to struct you can iterate through all the manifolds without such massive amounts of memory allocations as before. I think we can close this issue.

BTW. I also suggest to change all the IList references to List. As I mentioned above, using foreach operator over IList generates garbage. I know that we should depend on interfaces wherever possible but I think that this is one place, where exception is justified.

@Zolniu Zolniu closed this as completed May 16, 2015
@AndresTraks
Copy link
Owner

Ok, cool. Thanks!

I'll do some research on the IList enumeration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants