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

Fix Multi-Threaded Physics 2D crashes #48001

Closed
wants to merge 1 commit into from

Conversation

Listwon
Copy link
Contributor

@Listwon Listwon commented Apr 18, 2021

Fixes #29369, prevents crash in #29364 (but there are many other errors in log), possible fix for #35017 and #47489

I know I didn't add this to master first, but all test MRPs are for previous versions and privately I need this for 3.x first too.

I need some guidance how to rework this fix - I'm not sure if it's better to create separate data structures (as I did in map_mt.h) and replace them in critical places (area_2d_sw.h, area_2d_sw.cpp) or directly change existing structures as I did in self_list.h (maybe with addition of some directives or template arguments).

Also I'll appreciate any help testing this.

@Listwon Listwon requested review from a team as code owners April 18, 2021 13:36
@akien-mga akien-mga modified the milestones: 4.0, 3.4 Apr 18, 2021
@akien-mga akien-mga requested a review from RandomShaper April 18, 2021 14:57
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.

Thanks for working on this! Your contribution is welcome, but I wonder if this is the right approach in fixing these threading issues.

Multi-threading issues can be complex so team work is going to be important here :)

Thread-safe data structures:
Adding mutexes on single operations within map or self_list is not the most efficient way to solve multi-threading issues. A better approach could be to add a mutex in the code that uses the structure, so it can lock only once when needed and avoid to possibly stall multiple times.

About the specific issues in physics:
Maybe a mutex will be needed in some cases, but I'd like to check if simpler (and more efficient for performance) strategies can be used:

So I think the next steps can be to test these issues on master and identify the ones that are still problematic, so we can decide how to solve these cases individually.

About testing:
What have you been able to test/repro so far? Apart from testing the MRP from different issues, it seems you can compile godot with use_tsan=yes in order to enable thread sanitizer and catch specific threading issues (like the case from #47489). I haven't used it before, but maybe @qarmin can help if you have more questions.
I'll be happy to help testing once I have this set up as well.

@pouleyKetchoupp
Copy link
Contributor

Closing this PR as there's no update and it's not likely to be merged the way it's implemented.

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