Page MenuHomeWildfire Games

Repeating Tasks
Needs ReviewPublic

Authored by phosit on Jan 22 2023, 2:11 PM.

Details

Summary

We use futures to synchronize tasks. Currently tasks can only be function objects which then are executed in one pice (whitout pause).
This diff extends tasks to also include "repeating tasks". "repeating tasks" do after a delay re-invoke the function object untill an invokation returns Repeating::Result::Stop or the task is canceled externaly (via .CancelOrWait()).

TaskManager

To make it easier to write more "algorithms" the TaskManager should be as flexible as possible. It should not handle rescheduling.
The TaskManager has to be extendet to delay a task.
There is a new queue. Which holds a struct with a task and a timepoint. The timepoint coresponds to the time when the task should be executed. The queue is ordered (std::priority_queue) by the time when it should be executed first (chronological) first (queue order).
Every WorkerThread which is waiting on the m_ConditionVariable does waikup after some time (wait_for instead of wait).
Before checking for a TaskPriority::NORMAL task the new queue is checked. If the execution time of the first task is in the past that task is poped and executed.
The wakeup of the m_ConditionVariable could be adopted to the execution time of the first task (that's not implemented).

Future

If i make a RepeatingFuture type (which knows how to delay and repeat) it is complicated to send the signal to that RepeatingFuture. A thread would have to be waiting on the a condition_variable or the SharedState would have to store a refference to the RepeatingFuture which in turn would make it hard to make the RepeatingFuture moveable. The solution i came up with is that "the thing which knows how to repeat" is not the Future type but the PackagedTask. That does also enable me to reuse the existing Future.
There is a new ShareState which holds in addition to the function object the delay duration and the scheduler in which the task can be scheduled.
There is a new PackagedTask which (like the old PackagedTask) only holds a pointer to the SharedState. The reason that there is a new one is that It does (know how to) delay and repeat the task.
In my git branch the SharedState knows how to delay and repeat itself. So there is no extra PackagedTask.

Test Plan

There are tests, did I miss some?

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

phosit created this revision.Jan 22 2023, 2:11 PM

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

Debug:
    13>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(151): error C2143: syntax error: missing ';' before '<' [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\engine.vcxproj]
    13>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(151): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\engine.vcxproj]
    13>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(151): error C2238: unexpected token(s) preceding ';' [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\engine.vcxproj]
    14>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(151): error C2143: syntax error: missing ';' before '<' (compiling source file ..\..\..\source\graphics\MapGenerator.cpp) [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\graphics.vcxproj]
    14>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(151): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int (compiling source file ..\..\..\source\graphics\MapGenerator.cpp) [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\graphics.vcxproj]
    14>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(151): error C2238: unexpected token(s) preceding ';' (compiling source file ..\..\..\source\graphics\MapGenerator.cpp) [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\graphics.vcxproj]
    14>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(151): error C2143: syntax error: missing ';' before '<' (compiling source file ..\..\..\source\graphics\MapReader.cpp) [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\graphics.vcxproj]
    14>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(151): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int (compiling source file ..\..\..\source\graphics\MapReader.cpp) [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\graphics.vcxproj]
    14>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(151): error C2238: unexpected token(s) preceding ';' (compiling source file ..\..\..\source\graphics\MapReader.cpp) [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\graphics.vcxproj]
    13>e:\jenkins\workspace\vs2015-differential\source\ps\taskmanager.cpp(253): error C2511: 'void Threading::TaskManager::PushDelayedTask(std::chrono::system_clock::duration,Threading::QueueItem)': overloaded member function not found in 'Threading::TaskManager' [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\engine.vcxproj]
    11>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(151): error C2143: syntax error: missing ';' before '<' [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\simulation2.vcxproj]
    11>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(151): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\simulation2.vcxproj]
    11>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(151): error C2238: unexpected token(s) preceding ';' [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\simulation2.vcxproj]

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/7832/display/redirect

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

builderr-debug-gcc7.txt
In file included from ../../../source/ps/TaskManager.h:21:0,
                 from ../../../source/simulation2/components/CCmpPathfinder_Common.h:38,
                 from ../../../source/simulation2/components/CCmpPathfinder.cpp:25:
../../../source/ps/Future.h:151:28: error: 'Receiver' in namespace 'FutureSharedStateDetail' does not name a template type
   FutureSharedStateDetail::Receiver<void> m_Recv;
                            ^~~~~~~~
../../../source/ps/Future.h: In constructor 'Future<Ret>::Future(Scheduler&, Delay, Func)':
../../../source/ps/Future.h:294:60: error: 'Receiver' is not a member of 'FutureSharedStateDetail'
   m_SharedState = std::shared_ptr<FutureSharedStateDetail::Receiver<void>>(temp, &temp->m_Recv);
                                                            ^~~~~~~~
../../../source/ps/Future.h:294:73: error: template argument 1 is invalid
   m_SharedState = std::shared_ptr<FutureSharedStateDetail::Receiver<void>>(temp, &temp->m_Recv);
                                                                         ^~
make[1]: *** [simulation2.make:285: obj/simulation2_Debug/CCmpPathfinder.o] Error 1
make: *** [Makefile:109: simulation2] Error 2

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

phosit requested review of this revision.Jan 22 2023, 3:03 PM
phosit updated this revision to Diff 21441.Jan 22 2023, 6:30 PM
phosit edited the summary of this revision. (Show Details)

SharedState::m_SchedulFunc could became dangling. Now a copy is stored.
Repeating is a namespace now. There was no real reason to use a struct.

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

Debug:
    11>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(150): error C2143: syntax error: missing ';' before '<' [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\simulation2.vcxproj]
    11>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(150): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\simulation2.vcxproj]
    11>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(150): error C2238: unexpected token(s) preceding ';' [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\simulation2.vcxproj]
    14>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(150): error C2143: syntax error: missing ';' before '<' (compiling source file ..\..\..\source\graphics\MapReader.cpp) [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\graphics.vcxproj]
    14>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(150): error C2143: syntax error: missing ';' before '<' (compiling source file ..\..\..\source\graphics\MapGenerator.cpp) [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\graphics.vcxproj]
    14>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(150): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int (compiling source file ..\..\..\source\graphics\MapReader.cpp) [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\graphics.vcxproj]
    14>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(150): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int (compiling source file ..\..\..\source\graphics\MapGenerator.cpp) [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\graphics.vcxproj]
    14>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(150): error C2238: unexpected token(s) preceding ';' (compiling source file ..\..\..\source\graphics\MapReader.cpp) [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\graphics.vcxproj]
    14>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(150): error C2238: unexpected token(s) preceding ';' (compiling source file ..\..\..\source\graphics\MapGenerator.cpp) [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\graphics.vcxproj]
    12>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(150): error C2143: syntax error: missing ';' before '<' [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\engine.vcxproj]
    12>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(150): error C4430: missing type specifier - int assumed. Note: C++ does not support default-int [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\engine.vcxproj]
    12>e:\jenkins\workspace\vs2015-differential\source\ps\future.h(150): error C2238: unexpected token(s) preceding ';' [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\engine.vcxproj]
    12>e:\jenkins\workspace\vs2015-differential\source\ps\taskmanager.cpp(253): error C2511: 'void Threading::TaskManager::PushDelayedTask(std::chrono::system_clock::duration,Threading::QueueItem)': overloaded member function not found in 'Threading::TaskManager' [E:\Jenkins\workspace\vs2015-differential\build\workspaces\vs2017\engine.vcxproj]

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/7833/display/redirect

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

builderr-debug-macos.txt
In file included from ../../../source/simulation2/components/CCmpPathfinder.cpp:25:
In file included from ../../../source/simulation2/components/CCmpPathfinder_Common.h:38:
In file included from ../../../source/ps/TaskManager.h:21:
../../../source/ps/Future.h:150:27: error: no template named 'Receiver' in namespace 'FutureSharedStateDetail'
        FutureSharedStateDetail::Receiver<void> m_Recv;
        ~~~~~~~~~~~~~~~~~~~~~~~~~^
../../../source/ps/Future.h:293:60: error: no member named 'Receiver' in namespace 'FutureSharedStateDetail'
                m_SharedState = std::shared_ptr<FutureSharedStateDetail::Receiver<void>>(temp, &temp->m_Recv);
                                                ~~~~~~~~~~~~~~~~~~~~~~~~~^
../../../source/ps/Future.h:293:73: error: expected '(' for function-style cast or type construction
                m_SharedState = std::shared_ptr<FutureSharedStateDetail::Receiver<void>>(temp, &temp->m_Recv);
                                                                                  ~~~~^
3 errors generated.
make[1]: *** [obj/simulation2_Debug/CCmpPathfinder.o] Error 1
make: *** [simulation2] Error 2

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

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

builderr-debug-gcc7.txt
In file included from ../../../source/ps/TaskManager.h:21:0,
                 from ../../../source/simulation2/components/CCmpPathfinder_Common.h:38,
                 from ../../../source/simulation2/components/CCmpPathfinder.cpp:25:
../../../source/ps/Future.h:150:27: error: 'Receiver' in namespace 'FutureSharedStateDetail' does not name a template type
  FutureSharedStateDetail::Receiver<void> m_Recv;
                           ^~~~~~~~
../../../source/ps/Future.h: In constructor 'Future<Ret>::Future(Scheduler&, Delay, Func)':
../../../source/ps/Future.h:293:60: error: 'Receiver' is not a member of 'FutureSharedStateDetail'
   m_SharedState = std::shared_ptr<FutureSharedStateDetail::Receiver<void>>(temp, &temp->m_Recv);
                                                            ^~~~~~~~
../../../source/ps/Future.h:293:73: error: template argument 1 is invalid
   m_SharedState = std::shared_ptr<FutureSharedStateDetail::Receiver<void>>(temp, &temp->m_Recv);
                                                                         ^~
make[1]: *** [simulation2.make:285: obj/simulation2_Debug/CCmpPathfinder.o] Error 1
make: *** [Makefile:109: simulation2] Error 2

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

phosit updated this revision to Diff 21443.Jan 22 2023, 7:44 PM
phosit edited the summary of this revision. (Show Details)

Write TaskManager tests.
Change the WorkerThread wakeup, so it does actualy work and use less cpu-time

Stan added a reviewer: Itms.Jan 22 2023, 8:32 PM

SchedulFunc -> ScheduledFunction

source/ps/Future.h
146

missing #include.

192

missing #include.

239

missing #include.

source/ps/TaskManager.cpp
199

missing #include.

source/ps/tests/test_Future.h
151

missing #include.

158

missing #include.

168

missing #include.

204

missing #include.

source/ps/tests/test_TaskManager.h
121

missing #include.

124

missing #include. Maybe should use timer_time()

phosit marked 10 inline comments as done.Jan 22 2023, 9:26 PM
In D4907#209541, @Stan wrote:

SchedulFunc -> ScheduledFunction

It isn't the scheduled function. It's the function which has to be called to schedule something.

source/ps/Future.h
192

is_same_v is in <type_trates> which is already included.

source/ps/tests/test_Future.h
204

The part of std::literals i used is included with <chrono>

phosit updated this revision to Diff 21444.Jan 22 2023, 9:28 PM
phosit marked 2 inline comments as done.

Add some includes... Linted by Stan ;)

Stan added a comment.Jan 22 2023, 10:43 PM

SchedulerFunction then?

phosit updated this revision to Diff 21452.EditedJan 23 2023, 8:10 PM

Invert priority_queue order.
include utility where needed.
rename members of SharedState.

phosit updated this revision to Diff 21513.Feb 2 2023, 4:12 PM

More consistent naming.

phosit updated this revision to Diff 21739.Apr 19 2023, 7:10 PM
phosit edited the summary of this revision. (Show Details)

Remove the "dynamic delay". Because it's easier to review.

sera added a subscriber: sera.Apr 19 2023, 9:27 PM
sera added inline comments.
source/ps/TaskManager.cpp
199

Instruction? Somehow it feels m_DelayedTasks should be a collection of DelayedTask.

344

What's the rational for priority, after all they are delayed, would some more delay matter?

phosit added inline comments.Apr 19 2023, 10:14 PM
source/ps/TaskManager.cpp
199

To much things are called task.
I think i called it that way because i think of a task as the function object. An instruction is a task with the information when to execute.
I'm not realy sure anymore and will apply your suggestion.

344
  • It looks like there are only four "tasks" in the DelayedQueue. Even if all DelayedTasks are executed at the same time there are still some threads which can execute the other tasks (provided there are eight threads). The pathfinder in contrant might block the whole TaskManager.
  • If a DelayedTask is delayed to much the connection to the server might be lost. If a non-timed task is delayed there is only some lag.
  • As wraitii _always_ says: "time critical tasks are designed to work even if the thread pool is bussy." I hate that statement but it supports my implementation ;P.
sera added inline comments.Apr 20 2023, 10:53 AM
source/ps/TaskManager.cpp
199

You could also change the variable name if you prefer, tho Instruction feels a bit strange a pick.

Some other suggestions for a name if you feel neither is right: DelayingTaskWrapper or TaskDelayer both would work with with m_DelayedTasks

wraitii added inline comments.Apr 20 2023, 2:10 PM
source/ps/TaskManager.cpp
199

I'd call that an WorkOrder maybe, or a TaskOrder. Possibly a TaskTicket.
Agreed that Instruction feels like the wrong word though I get the idea.

The old SharedState and PackagedTask can't be used. I implemented similar types (there is some duplicate code). Some code should be moved to (the common) Receiver. That can be done before this diff.

I implemented that at my gitlab repo.

phosit edited the summary of this revision. (Show Details)May 1 2023, 10:33 PM
phosit edited the test plan for this revision. (Show Details)
phosit edited the summary of this revision. (Show Details)May 2 2023, 6:37 PM