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

Add multithreading #200

Closed
wants to merge 1 commit into from
Closed

Add multithreading #200

wants to merge 1 commit into from

Conversation

psincf
Copy link

@psincf psincf commented May 21, 2019

So this PR aim to add multithreading to nphysics.
This is still a draft, and a lot of things still need to be done.
I prefer to get some opinion before going further, in order to be sure we agree on the design choice.

So here is the actual choices made for this PR :

1. In Bodyset, body and ground are now locked by a RwLock (from parking_lot) :

  • When body or body_mut are called, It's returning a RwLockReadGuard and RwLockWriteGuard.
    This require rigid_bidy() and multibody() methods from world to be changed so I made them returning a MappedReadLockGuard (or MappedWriteLockGuard for the "mut" methods), in order to have downcast() working.

  • when add_body() is called it was returning a &mut to a body trait object. With this PR, it should return the RwLock. I changed it and made it return the BodyHandle instead, because I thought it may be better than directly manipulate the RwLock. I am not really sure about this!

  • One of the problem with RwLock, is that if you want to manipulate a body from your app, and let's say having a write lock, and then you call step(). If you forgot to unlock the body, it's going to block the app, and finally, you are going to have a lot of responsability when using nphysics!!
    A solution would be to make the RwLock internal to the body, and each time you call a method, it's internally acquiring the appropriate lock ( read or write ) and then releasing it when finished. The problem is that it would probably kill performance.

  • Another radically different alternative is to change N: RealField to N: Atomic<RealField> with the atomic crate for example. Then It would be totally non-blocking, and the RwLock problem would not exist. We could use Relaxed ordering for maximum performance and rely on fence for memory barrier.
    One possible issue would be for performance on weekly ordered architecture. I don't have experience on theses ones so I am not sure, but it seems that atomic operations are more expansive. How is it in reality? ( like on ARM for example)

2. A step is divided in two methods. The master, and the slaves.

  • The master function ( named step_multithread_master() here ) distribute and manage work made by the "slaves" ( step_multithread_slave() ).

  • Basically, the user clone a ref to the World struct between thread (with Arc for example), the main thread call the master function and the others call the slave one. In order to work with safe Rust, it need to entirely rely on interior mutability in order to safely share World between trade (With RwLock and/or Atomic like said upper).

  • Master and slaves communicate through MultithreadStep struct, and each step has his own. The struct is still very basic and can be improved (with Barrier instead of constantly loading atomic values in a loop for example)

  • nphysics itself doesn't manage threads.

3. Example in Testbed3d

  • Testbed3d has been changed to support multithreading with the possibility to choose the number of threads with set_thread_number(). If it's 0, then the original step() method is called.

Things That need to be done :

1. Use exclusively interior mutability in World with RwLock/Atomic

  • For the moment, only Bodyset has been rewritten. Because of that, unsafe is temporary used in testbed3d to make multithreading work.

2. Complete master and slave method

  • Only update_kinematics() and update_dynamics() have been done.

3. Reorganize step in smaller step ?

4. Rebase on top of CCD ?

  • Reading the blog of rustsim, you want to implement multithreading after ccd, so I can rebase it on top of the ccd branch if you want.

5. Make ncollide multithreaded

  • One possibility is to make the master/slave system identical in ncollide. This way, we can call the slave and master methods of ncollide in the respective methods of nphysics

6. Other suggestions ?

  • If you have other design ideas ? Suggestions ? Remark ?

@sebcrozet
Copy link
Member

sebcrozet commented Jun 13, 2019

Hi @psincf! Thank you for attempting to add multithreading and sorry, I've not had the time to review your PR yet in details. Hopefully I will have some time to look at it in detail within the next few weeks.

@psincf
Copy link
Author

psincf commented Jun 15, 2019

@sebcrozet Thanks for taking the time to reply! And no worries, there is no hurry :)

@psincf
Copy link
Author

psincf commented Oct 28, 2019

This PR seems no longer relevant, and is now really different from the actual master branch. Also I think that the design choice wasn't good.
I think it's better to close it, and let someone make a better proposal

@psincf psincf closed this Oct 28, 2019
@psincf psincf deleted the multithreading branch December 2, 2019 19:48
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

Successfully merging this pull request may close these issues.

2 participants