Skip to content

Improvements to memory handling for WorkerThreadPools #11201

Description

@BrianBHuynh

Describe the project you are working on

I am working on holding thursday sessions where I teach students in university (juniors and seniors) how to use github, as well as collaborate on a project in godot (https://github.com/BrianBHuynh/GameDev-lesson-team-project) which is sanctioned by the computer science department head for my university.

I am also working on a cute but super early stage vpet / social game (currently in accessibility stages) https://github.com/BrianBHuynh/Mieu-CatChat

I am also a frequent contributor to the Godot docs.

Because of the first and 3rd projects ease of use and documentation is quite important to me.

Describe the problem or limitation you are having in your project

Currently the implementation of the WorkerThreadPool is slightly unintuitive and hard to teach/use, especially from a documentation standpoint as the names of functions in the object can be misleading or uninformative. Currently memory for task is not automatically managed, and each task needs to manually be cleared/acknowledged/merged through a "wait" function.

There was an issue (godotengine/godot#84888, godotengine/godot#84899) posted around a year ago which was resolved by a documentation change due to lack of knowledge on how Worker ThreadPools work at the time, however even though the issue was resolved, I think implementation of threadpools can be more intuitive.

Currently the documentation officially states that

Warning: Every task must be waited for completion using wait_for_task_completion or wait_for_group_task_completion at some point so that any allocated resources inside the task can be cleaned up.

However this can be confusing as you must wait a task even if it is completed for memory to be cleared, which is unintuitive as for the WorkerThreadPool, the wait functions do not solely freeze the thread it is called from, but instead act as the main form of resource clearing as well!

Some issues to show that people do run into problems (before and after the documentation change): godotengine/godot#79069
godotengine/godot#84888
(after)
godotengine/godot#84899
godotengine/godot#99316 (mine)

As can be seen in 84899 there has been complaints about the current solution as having to call waits to clear resources "pretty much eliminate the ease of use of WorkerThreadPool (fire and forget)." and requires some active effort to manage memory and some people would expect task to happen automatically as there is no function which's name implies memory/resource management so a reading of the docs is needed to use this object intended to simplify the process of multi threading.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

I have multiple solutions which may make memory management via threads easier (and which I've gotten some progress into implementing)

  1. Add a function which has a simpler use, acting to merely clear resources and return true if the task is completed (Add clear_task_if_completed function to WorkerThreadPool godot#99316)
    OR
  2. Roll this functionality into the current is_task_completed and is_group_task_completed functions

Both of these solutions would make it so that there is a way to acknowledge the task, and also clear memory without calling a wait function (which may risk freezing a thread, or the main thread, if implemented badly). Also the second name would have a slightly more intuitive name and use case without the need for documentation reading (and it's existence implies that task need to be resource cleared).

  1. Add a auto clear function / bool which would automatically remove the task from memory once it is completed.

By implementing this in the engine, it will make it easier for programmers to manage memory if there is no need for acknowledgement. In the case when the completion of code does not need to be acknowledged (for example in the case of a save system which is both intensive and also should not be waited on).

  1. Rename wait to join, as suggested by the main programmer for WorkerThreadPools @RandomShaper which may put it more in line with the function of Joining threads. (brought up as a solution here Add clear_task_if_completed function to WorkerThreadPool godot#99316)

My suggested solution:
I think that a hybrid of the 3 solutions would make the WorkerThreadPools far more intuitive to use. By combining the is_task_completed checks with resource clearing (like in the wait functions) to allow for waitless resource management, adding a method to automatically clear memory for less essential task, and renaming the wait function, the worker threadpool would be far more intuitive to use.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

As the core engine should be kept as lean as possible, I have included the amount of lines AND loops included in each solution in theory (only for singular task, I have not looked at group task yet but they should be similar )

Implementing the 3 suggested solutions is estimated to add anywhere from 25 lines of extra code, to a low of 10 extra lines, although this is only for single task, and assuming similar implementation both group and single task will take anywhere from 20 to 50 extra lines of code.

solution 1 (lines added 27):
godotengine/godot#99316

solution 2 (lines added 6):
replace the current is_task_completed function with the clear_task_if_completed function code in solution 1, this adds only 6 more lines onto the current function that's already implemented.

solution 3 (aprox lines added 19):
Add these lines to the end of the _process_task internal function

if (p_task->auto_complete && p_task->completed && p_task->waiting_pool == 0 && p_task->waiting_user == 0) {
	tasks.erase(p_task->self);
	task_allocator.free(p_task);
}

Add this function

bool WorkerThreadPool::auto_clear_task(TaskID p_task_id) {
MutexLock task_lock(task_mutex);
Task **taskp = tasks.getptr(p_task_id);
if (!taskp) {
ERR_FAIL_V_MSG(false, "Invalid Task ID"); // Invalid task
}
Task *task = *taskp;
if (task->completed) {
if (task->waiting_pool == 0 && task->waiting_user == 0) {
tasks.erase(p_task_id);
task_allocator.free(task);
}
return true;
}
task->auto_complete = true;
return task->auto_complete;
}

NOTE: 14 of those lines for solution 3 can be removed IF willing to add a argument into the add task function, although this more intensive solution will work with all current code without breaking it.

Solution 4 (0 lines added): Just renaming function from wait to join which may be more accurate.

If this enhancement will not be used often, can it be worked around with a few lines of script?

This enhancement can be worked around in a few lines of code, however the current workaround can be clunky and hard to simply write/explain in the docs to someone without coding knowledge, and the usage of WorkerThreadPools is ideally to simplify the multithreading process for people not familiar and or unwilling with implementing it themselves with the Threads class.

Workaround for checking to see if a task is completed, and then clearing resources:
If is_task_completed(tid):
wait_for_task_completion(tid)

Note how if you want to prevent memory leaks, and no other code relies on acknowledging that your task is finished you must check to see if a task is completed, and then WAIT on said task completing (although it's already completed).

You also need to keep an array or list of active task as well, as task management is not automatically done, and there is no way to get a list of all the currently monitored tasks.

Is there a reason why this should be core and not an add-on in the asset library?

In worse case this could expand the core by ~50 lines in return for easier use of the worker threadpool and overall some performance improvements all around when using the worker threadpool. (Allows automatic memory management, or a way of managing memory without freezing the thread it is called from (which also calls 2 functions instead of 1)).

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions