Add clear_task_if_completed function to WorkerThreadPool
#99316
Conversation
AThousandShips
changed the title
clear_task_if_completed function to WorkerThreadPool
|
Just merged the commit since most of the test seemed to have been working after the small fix :D |
|
Ah I need to update the documentation, 1 second |
|
Why not go overboard, make a new function named |
core/object/worker_thread_pool.cpp
Outdated
| task_mutex.lock(); | ||
| Task **taskp = tasks.getptr(p_task_id); | ||
| if (!taskp) { | ||
| task_mutex.unlock(); |
There was a problem hiding this comment.
If the task has been already waited on, it will raise an error.
There was a problem hiding this comment.
Oh yeah I guess that lock isn't needed huh
There was a problem hiding this comment.
Just removed the locks!
|
I was thinking it'd be nice to have a way for the developer to manually clear a task (and acknowledge that it was finished via the bool) without having to call the wait function, but yeah I think it's a good idea to implement a auto clear task function like you suggested. Atleast for me I think having something like this would be nice in general though in the case the programmer needs to acknowledge the task being completed in a way which isn't as time sensitive as using a whole wait for. I'll work on implementing a auto clear function as well though |
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)
|
I'm not quite sure this adds significant value. On the one hand, the current API is designed to mimic joining threads. Being that the case, code using the threadpool would be designed in a way that the need for something like this is avoided in the first place. On the other hand, while there may be scenarios where this may be useful, given how wary we should be of growing the engine, let alone its core, there should be a proposal, backed by a high amount of real-world demand. Other approaches would be discussed there. Also, maybe other ideas such as renaming |
|
Another approach I was suggested by @RadiantUwU which may have more real world use would be the addition of a auto-clear or auto complete method, which would possibly do the same thing as this function but automatically (and without the return). Do you think that would be more useful since I've already been working on it slightly to see if I could write an implementation? I think that that may warant some extra consideration if this idea feels too frivolous of an addition The main purpose of this addition was to make the worker threadpool more user friendly anyways, do you think that adding a bool to toggle the current wait functions instead would be a possibility? (And also the rename) I think that having a way to clear the resources without risking a wait on the main thread (and possibly a more intuitive name) may make the worker threadpool easier to use in general! Please lmk what you think of either of those ideas since I can get working on them ASAP to see what you think! |
|
Another idea I just thought of was combining the is_completed function that was already there with the function I wrote, giving a bool (or possibly just clearing the memory on true) which would dictate if the task would be cleared from memory as well! Wanted to write it here as well since I just thought of it haha |
|
All those ideas may have merit, but the point about needing real-world examples and discussing alternatives and roadmap in a proposal stands. |
|
Alright! I'll start working on writing a proposal for the ideas I have and see what people think of them then! Thankyou for reading over what I had so far (I know it's alot haha) |
Currently every task must be waited on (wait_for_task_completion) for their resources to be removed from memory (#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)