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

Compile bullet with threadsafe switch on #53183

Merged
merged 1 commit into from
Sep 29, 2021

Conversation

m4nu3lf
Copy link
Contributor

@m4nu3lf m4nu3lf commented Sep 28, 2021

Compile Bullet with the BT_THREADSAFE switch so that it is thread-safe to raycast from multiple threads (as long as the physics engine is not stepping).

I'm using this feature in my project to implement projectile rayscan and visibility checks.
Making this for 3.x first as Bullet seems to not be compiled yet when compiling from master.

@m4nu3lf m4nu3lf requested a review from a team as a code owner September 28, 2021 15:23
@Calinou Calinou added this to the 3.4 milestone Sep 28, 2021
@Calinou
Copy link
Member

Calinou commented Sep 28, 2021

What downsides does the BT_THREADSAFE flag entail? Does it lower performance or something like that?

@m4nu3lf
Copy link
Contributor Author

m4nu3lf commented Sep 28, 2021

According to this benchmark, there is around a ~3% penalty in single-thread performances due to this change.
The benchmark is not totally stable, but I ran it 5 times on each build.
Keep in mind that the benchmark is on a very physics-intensive scene with lots of rigid bodies and raycasts.
I think that people working with this kind of complex scene would have more to gain by moving all raycast logic to a thread pool (like I'm doing in my project), but if we really don't want to have this penalty we could add a compilation switch maybe.

Side note: I noticed by looking at the code that Bullet has a hard limit on the number of threads that can access it as it keeps track of them, and it is now set to 64.

@m4nu3lf m4nu3lf force-pushed the threadsafe_bullet_3.x branch from 358321f to 6f81c21 Compare September 28, 2021 16:21
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a great change, thanks!

Having a 3% tradeoff on single-threaded raycasts doesn't seem too bad, given that in extensive usage you can now use threads to make it much faster.

I've run some general tests with the physics simulation, and didn't spot any performance drop or other issue.

If anything, it seems adding/removing objects in the broadphase is a little bit faster, but I have no idea why :)

@m4nu3lf
Copy link
Contributor Author

m4nu3lf commented Sep 28, 2021

Looks like a great change, thanks!

Having a 3% tradeoff on single-threaded raycasts doesn't seem too bad, given that in extensive usage you can now use threads to make it much faster.

I've run some general tests with the physics simulation, and didn't spot any performance drop or other issue.

If anything, it seems adding/removing objects in the broadphase is a little bit faster, but I have no idea why :)

Keep in mind that I only tested it with parallel raycasting. You can have many threads doing raycast at the same time (which is what I do) but that can't happen during the physics engine update. To support that, I have a node that runs last and accumulates requests of executing tasks from other nodes. Then, when it's its turn to run _physics_process(), it sends all the tasks to a thread pool and waits for all of them to finish before letting the physics server step.

I know there are compilation macros to enable parallel constraint resolution (which I think also require this flag), but that requires more changes to the glue code. Maybe I'll give it a try to enable that too .

@pouleyKetchoupp
Copy link
Contributor

I know there are compilation macros to enable parallel constraint resolution (which I think also require this flag), but that requires more changes to the glue code. Maybe I'll give it a try to enable that too .

There was an effort in the past to try and enable multi-threaded physics in Bullet, but it got stalled because of lack of time, after the first results didn't confirm the performance improvements we expected.

This is @fire's PR, it should be good to start testing if you're interested:
#40073

@akien-mga akien-mga changed the title Compile bullet with threasafe switch on Compile bullet with threadsafe switch on Sep 29, 2021
@akien-mga akien-mga merged commit fba9fb2 into godotengine:3.x Sep 29, 2021
@akien-mga
Copy link
Member

Thanks!

@pouleyKetchoupp
Copy link
Contributor

Actually, even if Bullet is disabled on master, it would be good to port this change to keep branches in sync. Would you like to take care of it @m4nu3lf? Otherwise I don't mind doing it.

@akien-mga
Copy link
Member

I've included a cherry-pick of this commit in this PR for master: #48299.

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

Successfully merging this pull request may close these issues.

4 participants