-
-
Notifications
You must be signed in to change notification settings - Fork 22k
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
WorkerThreadPool leaks memory and occasionally crashes #84888
Comments
Confirmed in 4.2-beta6 on Linux. |
You have to wait for WorkerThreadPool tasks at some point so that WorkerThreadPool has a chance to clean up any used resources. In this case, use
However, the documentation doesn't directly mention this, so I'll add a documentation tag. Both I can't repro the crashing, which might be worth making a separate issue if you can repro it. Maybe you have to wait a lot longer for operating system thread, mutex etc. resources to reach their process limit or something like that? The limits tend to be quite high, so in normal use WorkerThreadPool should not crash very easily. |
Tested using Godot v4.2.beta (6415006) - Windows 10.0.19045 - GLES3 (Compatibility) - NVIDIA GeForce RTX 3060 (NVIDIA; 31.0.15.3619) - 11th Gen Intel(R) Core(TM) i7-11700K @ 3.60GHz (16 Threads), it didn't crash too but the memory leak is obvious. I guess the crash may related to OS or something else. |
Asking for clarification here, by "every task should be waited at some point" does the task of waiting (WorkerThreadPool.wait_for_task_completion()) itself clear up the resources, or would waiting enough time (i.e not a wait function) make the threads clearup resources ? I'm wondering because having a function which pauses the thread that calls this method seems to negate the benefit of using that thread if the following code does not depend on the task being completed (i.e saving a file) |
Adds clear_task_if_completed function to worker threadpools Currently every task must be waited on (wait_for_task_completion) for their resources to be removed from memory (godotengine#84888). This is addressed in the docs however said solution may not always be intuitive or ideal as the function known as "wait_for_task_completion" is used even in cases where no waiting is necessary as it does the double duty of waiting, and also clearing memory. While it makes sense that if you wait for a task, and it finishes, it should be cleared from memory, it is not always intuitive that you HAVE to wait for a task that you know is completed (via is_task_completed) to prevent a memory leak. This can currently be circumvented by using an if statement (if is_task_completed: wait_for_task_completion) however said usage is ugly and also unintuitive. This commit adds a function which checks if the task is completed, and then returns true and clears resources if so. Essentially doing the same job as wait_for_task_completion if already completed, HOWEVER not binding the main thread (or the thread it is called from) in the case the task isn't. This allows for resource clearing without accidentally pausing the entire thread if not completed, in the case of a non essential functions such as saving (which can be allowed to operate in the background without pausing the entire game/main thread)
Godot version
v4.2.beta6.official [6415006]
System information
Godot v4.2.beta6 - Windows 10.0.19045 - Vulkan (Mobile) - dedicated NVIDIA RTX A2000 Laptop GPU (NVIDIA; 31.0.15.2737) - 11th Gen Intel(R) Core(TM) i7-11850H @ 2.50GHz (16 Threads)
Issue description
Adding tasks to the WorkerThreadPool leaks memory and occasionally crashes.
While the memory leak is very reproducible, the crash only occurs very sporadically.
Running the same tasks using dedicated threads or a custom thread pool does not leak memory.
This is not a regression, the same behavior occurs all the way back to v4.0.2.stable.official [7a0977c] and presumably earlier.
Steps to reproduce
Run a scene containing a node with this script, view the static memory monitor in the Debugger pane, then click the mouse in the game window and watch the memory usage ramp up.
Minimal reproduction project
Trivial to reproduce without a dedicated project.
The text was updated successfully, but these errors were encountered: