Page MenuHomeWildfire Games

ThreadPool extension -> Timer for recurrent task & adapt netClient.
Needs ReviewPublic

Authored by wraitii on Apr 20 2021, 11:17 AM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Extends D3848.
The idea is that some tasks, such as the netclient, need to run regularly but are very fast to execute, so a dedicated thread would be "mostly idle". If we have several "mostly idle" threads, they'll end up waking up randomly, pessimising the designe of the thread pool.

Instead, this implements a single 'Timer' thread, that wakes up at fixed intervals (intended to be 'short enough'). There, very fast 'dispatch' tasks are run, that may push work to other workers. The timer thread is intended to go back to sleep ASAP.

The 'recrrent tasks' push work on the other workers, keeping the 'contract' that we have one thread-per-core & efficient resource usage, and the single timer-thread avoids having multiple mostly-idle-threads waking at random times.

This can also easily be adapted for sound & the net server. I think it's better than dedicated threads, since those threads still mostly sleep.
A GPU thread would probably need its own worker, so that's handled by a different use case.

Test Plan

Agree with the concept/design.

Event Timeline

wraitii created this revision.Apr 20 2021, 11:17 AM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/4012/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/5720/display/redirect

Stan added a subscriber: Stan.Apr 20 2021, 12:17 PM
Stan added inline comments.
source/ps/ThreadPool.cpp
480–483

Use timer_time() instead of std::chrono as it was made for it, and I think there are some implem issues with regards to performance with it on windows.

wraitii added inline comments.Apr 20 2021, 4:50 PM
source/ps/ThreadPool.cpp
480–483

Can you source that?

I have to include <chrono> for condition_variable anyways, so I'd rather use it in general.

wraitii added inline comments.Apr 20 2021, 5:56 PM
source/ps/ThreadPool.cpp
480–483

I'm using wait_for which takes chrono types.

These 3 links seem outdated / not relevant for our compilers.

Stan added a comment.Jun 3 2021, 5:59 PM

This can definitely be used by the soundmanager too.

Else it would hog a thread like so with pauses from 500ms default to 50ms stress. (This is combat demo huge) Might also help to simplify the weird logic with the distress time to 500ms. (Takes about 1ms on my machine to do one loop)


Yeah I have a diff for that. I'll rebase those for A26

Hey @wraitii are you planning on cleaning up and landing these changes soon? Asking since I'd like to pull in changes with the AddRecurrentTask API and can either wait for this to land, or put up a diff with an updated version of TimerThread :)

Hey @wraitii are you planning on cleaning up and landing these changes soon? Asking since I'd like to pull in changes with the AddRecurrentTask API and can either wait for this to land, or put up a diff with an updated version of TimerThread :)

I'm not really planning to land this soon (because I don't have any real bandwidth). You can 'commandeer' this diff if you want, no need to upload a new one however :)

phosit added a subscriber: phosit.Oct 3 2022, 7:21 PM

I have a different idea:
A ThreadPool::runAfter(timePoint, timedTask) which pushes the timedTask to a new queue priority_queue<pair<timePoint, timedTask>>. Ordered only by .first.
Since it is a single(cheap) comparison timedTasks.top().first < now multiple/every workerThread could check.
So there is no need for a extra timerThread and there is no restriction in how long the task can take.
ThreadPool::addRecurrentTask can be implemented in terms of ThreadPool::runAfter:

curl_multi(https://curl.se/libcurl/c/curl_multi_timeout.html) for #968 does need something like ThreadPool::runAfter

I have a different idea:
A ThreadPool::runAfter(timePoint, timedTask) which pushes the timedTask to a new queue priority_queue<pair<timePoint, timedTask>>. Ordered only by .first.
So there is no need for a extra timerThread and there is no restriction in how long the task can take.

Part of the point of my timer thread is to try to use woken threads, to avoid waking up costs. Would your idea give the same benefit ?

phosit added a comment.EditedOct 15 2022, 12:12 PM

I have a different idea:
A ThreadPool::runAfter(timePoint, timedTask) which pushes the timedTask to a new queue priority_queue<pair<timePoint, timedTask>>. Ordered only by .first.
So there is no need for a extra timerThread and there is no restriction in how long the task can take.

Part of the point of my timer thread is to try to use woken threads, to avoid waking up costs. Would your idea give the same benefit ?

I don't understand your comment. To avoid wake up costs you restrict how long the task can take or to avoid wake up costs you introduce a new thread?

I don't understand your comment. To avoid wake up costs you restrict how long the task can take or to avoid wake up costs you introduce a new thread?

The point of the Timer-thread is to wake up once, process all the 'quick tasks' in a row, then sleep for [interval]ms again, to maximise throughput. This breaks down obviously if a task takes much more time than [interval]

The core difference is that there is a pirority queue of timed tasks instead of a pool of repeating tasks.
My idea can be implemented either way. (Using the worker threads or make a timed thread)

Thad said, for now i'm not in favor of the timed thread. The worker threads are mostly idle nobody would notice if they wake up every 10ms.

Stan added a comment.Oct 16 2022, 12:31 PM

Might notice it on the very slow hardware running the game.