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

Scene testruns with threading/worker_pool/max_threads set to 0 get stuck after recent WorkerThreadPool changes #77131

Closed
smix8 opened this issue May 16, 2023 · 8 comments · Fixed by #78845

Comments

@smix8
Copy link
Contributor

smix8 commented May 16, 2023

Godot version

508a5bf

System information

Windows 10, Intel i9-11900K

Issue description

Godot runs fine in the Editor but any test run of a (3D) scene freezes the window up.
Even a test scene with just a single Node3D freezes.

All the test project runs fine with slightly older versions of master / Godot 4.1.

There are no errors but after bisecting it the problem points at changes from pr #76945 so properly some kind of thread deadlock.

bisect_result

edit

Seems to be caused by ProjectSettings threading/worker_pool/max_threads set to 0 and shader_compiler/shader_cache/enabled=false

Steps to reproduce

set ProjectSettings threading/worker_pool/max_threads to 0 and shader_compiler/shader_cache/enabled=false and testrun a scene from the Editor.

Minimal reproduction project

WorkerThreadPoolIssue.zip

@RandomShaper
Copy link
Member

Despite how easy it seems to be possible to reproduce it, please share an MRP and commit hash so I can be very sure I'm debugging the exact situation.

@smix8
Copy link
Contributor Author

smix8 commented May 16, 2023

I think I found the real cause of the issue.

For some reason in all existing test projects that I tried the ProjectSettings had the threading/worker_pool/max_threads set to 0. Returning it to the default -1 fixed the issue. I did not notice it before cause the AdvancedSettings was still toggled off.

@RandomShaper
Copy link
Member

I had a hunch about project settings. Before my PR WorkerThreadPool settings were being ignored.

@smix8
Copy link
Contributor Author

smix8 commented May 16, 2023

Does it still make sense to allow this property to be set to 0 now that even the servers are switching to an internal use of the WorkerThreadPool?

@smix8
Copy link
Contributor Author

smix8 commented May 16, 2023

Added MRP.
The unhealthy combination of WorkerThreadPool set to zero with shader cache disabled were the ProjectSettings that made the testruns stuck.

@RandomShaper
Copy link
Member

Does it still make sense to allow this property to be set to 0 now that even the servers are switching to an internal use of the WorkerThreadPool?

Upcoming PRs are going to fix that. At least I know of one of mine and one of @myaaaaaaaaa.

@akien-mga akien-mga changed the title Scene testruns get stuck on Windows after recent WorkerThreadPool changes Scene testruns with threading/worker_pool/max_threads set to 0 get stuck after recent WorkerThreadPool changes Jun 22, 2023
@akien-mga
Copy link
Member

I can confirm the issue using the MRP in the latest master branch, on Linux.

Should we prevent setting max_threads to 0 until this is fixed?

@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 22, 2023
@myaaaaaaaaa
Copy link
Contributor

myaaaaaaaaa commented Jun 22, 2023

Should we prevent setting max_threads to 0 until this is fixed?

See the below pseudocode for a suggestion on how to accomplish this

diff --git a/core/object/worker_thread_pool.cpp b/core/object/worker_thread_pool.cpp
index d285be3e70..660a624a43 100644
--- a/core/object/worker_thread_pool.cpp
+++ b/core/object/worker_thread_pool.cpp
@@ -528,18 +528,21 @@ void WorkerThreadPool::wait_for_group_task_completion(GroupID p_group) {
 	}
 
 	task_mutex.lock(); // This mutex is needed when Physics 2D and/or 3D is selected to run on a separate thread.
 	groups.erase(p_group);
 	task_mutex.unlock();
 }
 
 void WorkerThreadPool::init(int p_thread_count, bool p_use_native_threads_low_priority, float p_low_priority_task_ratio) {
 	ERR_FAIL_COND(threads.size() > 0);
+	if (p_thread_count == 0) {
+		p_thread_count = 1;
+	}
 	if (p_thread_count < 0) {
 		p_thread_count = OS::get_singleton()->get_default_thread_pool_size();
 	}
 
 	if (p_use_native_threads_low_priority) {
 		max_low_priority_threads = 0;
 	} else {
 		max_low_priority_threads = CLAMP(p_thread_count * p_low_priority_task_ratio, 1, p_thread_count - 1);
 	}

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

Successfully merging a pull request may close this issue.

4 participants