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

BVH - thread safety option #48892

Merged
merged 1 commit into from
Jul 22, 2021
Merged

BVH - thread safety option #48892

merged 1 commit into from
Jul 22, 2021

Conversation

lawnjelly
Copy link
Member

@lawnjelly lawnjelly commented May 20, 2021

Added optional thread safe version through template argument and runtime switch, that wraps access with a mutex.

Related to #48749
Related to #48790

This temporarily also adds a project setting to turn on and off use of the mutex to hopefully allow us to test whether the bug is to do with threading. The project setting is rendering/threads/thread_safe_bvh and defaults to off.

On suggestion from @pouleyKetchoupp I've also added a try_lock so it should print a warning if it detects a contended situation (which would indicate that thread safety was needed).

Notes

  • As I can't replicate the BVH bug on my machine I've made this PR to try and fix / track down the source of the bug
  • I don't seem to get any significant slowdown with thread_safe_bvh on my linux system. It could be that uncontended mutexes are very cheap on my system, although I can't vouch for this on other platforms.
  • We could also use other alternatives to a mutex, I'm not an expert in this area so would welcome any improvements.
  • Using the try_lock I can't seem to get a contended lock on my system with the project from the issue (or any other). So hopefully the bug is something simpler, and we won't need to use a mutex in production. It still needs testing on windows, or by someone who can reproduce the bug.
  • As the render bvh and the physics bvh are in servers, hopefully the existing thread protection will prevent problems, but this PR should help us find out whether this is the case.

Update

The default value for the thread_safe_bvh is now off (if we introduce it set to on, we may never find out whether thread safety was the cause of the problems).

It now operates on both the rendering BVH, and the physics BVHs for 3d and 2d. The project setting name is still the same. I'm unsure a little about having it still as rendering/threads/thread_safe_bvh for this reason, but there doesn't seem to be a sensible section to put it into. It may well be a temporary setting - if we discover that thread safety is required, we should probably leave it on in all cases and remove the option, and vice versa if it is not the cause of the bug.

Instructions for Testers

  • You can download the builds to try, from the section at the bottom of this PR thread that says 'All checks have passed' with a green arrow. If you click on show all checks, choose the section for your platform, you are taken to a page where you can download the artifact (build) from a button / listbox in the top right corner.
  • Be sure to try the build with and without the rendering/threads/thread_safe_bvh set to on. Please post here whether it fixes the visual bugs with either thread_safe_bvh switched on or off, and especially whether you see an error / warning message Info : multithread BVH access detected (benign).

Added optional thread safe version through template argument and runtime switch, that wraps access with a mutex.
@lawnjelly lawnjelly changed the title BVH - thread safety option and improvements BVH - thread safety option May 25, 2021
@lawnjelly
Copy link
Member Author

lawnjelly commented May 25, 2021

Update

After extensive testing neither myself or @pouleyKetchoupp has seen contention for threads in linux or windows. So it could well be that the existing thread protection from using rendering and physics through servers is working correctly.

We still can't discount it as the cause of the linked issues, as we have been unable to replicate them. But the lack of evidence does move it down from number one suspect spot. As a result of this I've split the other bug fix into a separate PR, #49057.

Until we pinpoint the problem, it still may be worth having this available in one of the betas for testing. Or at least having those affected able to download the artifacts from here in order to try.

@akien-mga akien-mga merged commit 801205b into godotengine:3.x Jul 22, 2021
@akien-mga
Copy link
Member

Thanks!

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.

3 participants