-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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 multiple issues in WorkerThreadPool
#76945
Conversation
Doesn't seem too problematic. But yes, it will be mentioned in the changelog. |
bool low_priority_use_system_threads = GLOBAL_GET("threading/worker_pool/use_system_threads_for_low_priority_tasks"); | ||
float low_property_ratio = GLOBAL_GET("threading/worker_pool/low_priority_thread_ratio"); | ||
|
||
if (editor || project_manager) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've force-pushed to amend this check. (Formerly, it was calling the getters in the OS
singleton, but they still return false
unconditionally at this point.)
Sorry for the pushes, mark as draft, etc. I had just found an issue and thought it was a regression from this PR, but it isn't. I will just fix it in another PR, which may take more time. |
Fantastic work! |
This seems to be causing a deadlock when running unit tests
I cancelled the CI builds where the 3 jobs running unit tests had been stuck since more than 40 min. |
- Fix project settings being ignored. - Made usages of `native_thread_allocator` thread-safe. - Remove redundant thread-safety from `low_priority_threads_used`, `exit_threads`. - Fix deadlock due to unintended extra lock of `task_mutex`.
Quoting myself from RC:
|
How late can Specifically, if it can be initialized after Line 2441 in fd4a06c
|
Good question! And, conversely, how early can the other call happen? Maybe it can just be found out empirically, by watching when the pool is first used by some component. |
Alternatively, This would make It would also make it possible to unit test different |
@myaaaaaaaaa, that can work. Could you please incorporate that change to your PR? Let's say that's a related but separate issue than the ones this PR aims to fix and it shouldn't take much longer to be merged. Aside, there should be a way to assert that the thread pool has been properly initialized with threads by some point. For instance, this PR was breaking tests by failing to initialize it because there would be zero threads to run the tasks, but at least we could realize. With the changes we are considering, the test cases would just happily run all the tasks on the main thread, which is not what they are meant to do. |
Thanks! |
native_thread_allocator
thread-safe.low_priority_threads_used
,exit_threads
.task_mutex
.NOTE: I have a concern that by cherry-picking this into 4.0 the users that tweaked
threading/worker_pool/*
settings will now get a behavior change. It's the correct thing to happen, but maybe too surprising for 4.0, or at least warrants a note in the release notes.UPDATE: For the records, this may also fix some hard to reproduce crash related to a thread object in one of the projects used to test #74405, that worked fine otherwise.