Changeset View
Standalone View
source/ps/ThreadPool.h
- This file was added.
/* Copyright (C) 2021 Wildfire Games. | |||||||||||||||
* This file is part of 0 A.D. | |||||||||||||||
* | |||||||||||||||
* 0 A.D. is free software: you can redistribute it and/or modify | |||||||||||||||
* it under the terms of the GNU General Public License as published by | |||||||||||||||
* the Free Software Foundation, either version 2 of the License, or | |||||||||||||||
* (at your option) any later version. | |||||||||||||||
* | |||||||||||||||
* 0 A.D. is distributed in the hope that it will be useful, | |||||||||||||||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | |||||||||||||||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | |||||||||||||||
* GNU General Public License for more details. | |||||||||||||||
* | |||||||||||||||
* You should have received a copy of the GNU General Public License | |||||||||||||||
* along with 0 A.D. If not, see <http://www.gnu.org/licenses/>. | |||||||||||||||
*/ | |||||||||||||||
#ifndef INCLUDED_THREADPOOL | |||||||||||||||
#define INCLUDED_THREADPOOL | |||||||||||||||
#include "ps/Future.h" | |||||||||||||||
#include <memory> | |||||||||||||||
StanUnsubmitted Not Done Inline Actions
Stan: | |||||||||||||||
Not Done Inline ActionsStill. Can some of the below classes be marked final? they don't seem overridable anyway. Stan: Still.
Can some of the below classes be marked final? they don't seem overridable anyway. | |||||||||||||||
#include <vector> | |||||||||||||||
Not Done Inline Actions
Stan: | |||||||||||||||
class TestThreadPool; | |||||||||||||||
class CConfigDB; | |||||||||||||||
namespace ThreadPool | |||||||||||||||
{ | |||||||||||||||
Not Done Inline ActionsI'm not sure that it should be called ThreadPool. Because I think that TaskManager intersects with ThreadPool, but ThreadPool doesn't include the whole TaskManager. vladislavbelov: I'm not sure that it should be called `ThreadPool`. Because I think that `TaskManager`… | |||||||||||||||
Not Done Inline ActionsWhat's about that? vladislavbelov: What's about that? | |||||||||||||||
class TaskManager; | |||||||||||||||
class WorkerThread; | |||||||||||||||
enum class Priority | |||||||||||||||
{ | |||||||||||||||
Not Done Inline ActionsTaskPriority. vladislavbelov: `TaskPriority`. | |||||||||||||||
NORMAL, | |||||||||||||||
LOW | |||||||||||||||
}; | |||||||||||||||
/** | |||||||||||||||
* Execute work on the global queue. | |||||||||||||||
* The low-priority queue has a differently-typed executor to highlight the different | |||||||||||||||
* starvation guarantees. | |||||||||||||||
*/ | |||||||||||||||
Not Done Inline ActionsWhat's the point of making a class for it? Why not using WorkerAffinity = ...;? Also affinity means subset not a single element in other words affinity is a mask. Also in our case WorkerAffinity is useless for performance, it helps only for a profiler-like initializations than can be done in thread`s init. vladislavbelov: What's the point of making a class for it? Why not `using WorkerAffinity = ...;`? Also affinity… | |||||||||||||||
template<Priority Priority = Priority::NORMAL> | |||||||||||||||
class GlobalExecutor final : public IExecutor | |||||||||||||||
{ | |||||||||||||||
friend class TaskManager; | |||||||||||||||
protected: | |||||||||||||||
GlobalExecutor(TaskManager& tm) : m_TaskManager(tm) {} | |||||||||||||||
virtual void ExecuteTask(std::function<void()>&& task) override; | |||||||||||||||
virtual std::shared_ptr<IExecutor> Clone() const override { return std::make_shared<GlobalExecutor>(*this); }; | |||||||||||||||
TaskManager& m_TaskManager; | |||||||||||||||
}; | |||||||||||||||
/** | |||||||||||||||
* Execute work on a particular worker's queue. This allows thread-affinity for some tasks. | |||||||||||||||
*/ | |||||||||||||||
class ThreadExecutor final : public IExecutor | |||||||||||||||
{ | |||||||||||||||
friend class EachThreadExecutor; | |||||||||||||||
friend class TaskManager; | |||||||||||||||
protected: | |||||||||||||||
Done Inline Actions. vladislavbelov: . | |||||||||||||||
ThreadExecutor(WorkerThread& worker) : m_Worker(worker) {} | |||||||||||||||
virtual void ExecuteTask(std::function<void()>&& task) override; | |||||||||||||||
virtual std::shared_ptr<IExecutor> Clone() const override { return std::make_shared<ThreadExecutor>(*this); }; | |||||||||||||||
WorkerThread& m_Worker; | |||||||||||||||
Done Inline ActionsThe same should be for copying then. vladislavbelov: The same should be for copying then. | |||||||||||||||
}; | |||||||||||||||
/** | |||||||||||||||
* Provides a convenient interface to iterating all worker threads. | |||||||||||||||
*/ | |||||||||||||||
class EachThreadExecutor | |||||||||||||||
{ | |||||||||||||||
friend class TaskManager; | |||||||||||||||
Not Done Inline ActionsWhy std::list over vector / array Stan: Why std::list over vector / array
Does it make sense to use custom allocators for those ? | |||||||||||||||
Done Inline Actionsstd::vector cannot use non-movable objects for now, and array is constant-size. I don't think custom allocators particularly make sense. wraitii: std::vector cannot use non-movable objects for now, and array is constant-size.
I don't think… | |||||||||||||||
public: | |||||||||||||||
std::vector<ThreadExecutor>::iterator begin() { return m_Executors.begin(); } | |||||||||||||||
std::vector<ThreadExecutor>::iterator end() { return m_Executors.end(); } | |||||||||||||||
private: | |||||||||||||||
std::vector<ThreadExecutor> m_Executors; | |||||||||||||||
}; | |||||||||||||||
/** | |||||||||||||||
* The task manager class is the meat of the thread pool. | |||||||||||||||
* It creates all worker threads on construction. | |||||||||||||||
* To push work, query the appropriate executor. | |||||||||||||||
* See implementation for additional comments. | |||||||||||||||
Not Done Inline ActionsThat should not be public. vladislavbelov: That should not be public. | |||||||||||||||
*/ | |||||||||||||||
class TaskManager | |||||||||||||||
{ | |||||||||||||||
friend class WorkerThread; | |||||||||||||||
template<Priority Priority> | |||||||||||||||
friend class GlobalExecutor; | |||||||||||||||
public: | |||||||||||||||
TaskManager(); | |||||||||||||||
TaskManager(size_t numberOfWorkers); | |||||||||||||||
~TaskManager(); | |||||||||||||||
TaskManager(const TaskManager&) = delete; | |||||||||||||||
TaskManager(TaskManager&&) = delete; | |||||||||||||||
static void Initialise(); | |||||||||||||||
static TaskManager& Instance(); | |||||||||||||||
/** | |||||||||||||||
* Clears all tasks from the queue. This blocks on started tasks. | |||||||||||||||
*/ | |||||||||||||||
void ClearQueue(); | |||||||||||||||
Not Done Inline ActionsCancelTasks or ClearQueue. vladislavbelov: `CancelTasks` or `ClearQueue`. | |||||||||||||||
/** | |||||||||||||||
* @return the number of threaded workers available. | |||||||||||||||
*/ | |||||||||||||||
size_t GetNbOfWorkers() const; | |||||||||||||||
/** | |||||||||||||||
* The global executor assigns work to the global queue, not a specific worker. | |||||||||||||||
*/ | |||||||||||||||
GlobalExecutor<Priority::NORMAL> GetExecutor(); | |||||||||||||||
Not Done Inline Actionstypo? Stan: typo? | |||||||||||||||
GlobalExecutor<Priority::LOW> GetLowPriorityExecutor(); | |||||||||||||||
Not Done Inline ActionsIt's not a TaskManager responsibility to provide executors. TaskManager by its interface works only with tasks. A task executor shouldn't retrieved when the task is ready to execute and not when it's submitted. vladislavbelov: It's not a `TaskManager` responsibility to provide executors. `TaskManager` by its interface… | |||||||||||||||
Done Inline ActionsSee comment above on IExecutor - I need some more info on this I think.
So what I called a 'worker' here is what you call a 'task executor', right? But my IExecutor class is intended to be the lightweight handle-to-a-worker from e.g. http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p0443r12.html , not an 'execution context'. My design has the client code get an executor from the taskManager/thread pool (I agree that it makes the name a bit confusing, but perhaps we didn't understand the classes to do the same in the first place), then push work via that. The executor system is how I implement thread affinity and priority. I could skip that and just push work via the task manager directly - I didn't do it because it seemed the C++ standard would go more flexible, and I wanted the concepts here to be vaguely replaceable with standard ones someday. Can you clarify what you would have me change? wraitii: See comment above on IExecutor - I need some more info on this I think.
> A task executor… | |||||||||||||||
Not Done Inline ActionsI'd prefer to not have IExecutor at all. It's just an additional step for the code client. A simpler task manager can be extended to a more flexible one. But I don't think we will ever need it. We don't need to solve any possible problem. vladislavbelov: I'd prefer to not have `IExecutor` at all. It's just an additional step for the code client. A… | |||||||||||||||
/** | |||||||||||||||
* Assigns work to a specific worker. | |||||||||||||||
* Note that this is not guaranteed to return all workers if called repeatedly | |||||||||||||||
* (see notes on starvation in the implementation). | |||||||||||||||
* Iterate over GetAllWorkers if that's the target behaviour. | |||||||||||||||
*/ | |||||||||||||||
ThreadExecutor GetWorker(); | |||||||||||||||
/** | |||||||||||||||
* Returns an executor that can be used to start (optionally different) work on (optionally all) threads. | |||||||||||||||
*/ | |||||||||||||||
EachThreadExecutor& GetAllWorkers(); | |||||||||||||||
Not Done Inline ActionsMethods for testing shouldn't be in a public interface. vladislavbelov: Methods for testing shouldn't be in a public interface. | |||||||||||||||
Done Inline ActionsThis is also not for testing, but used for e.g. sending work from the pathfinder, so we can push work to each thread. Edit: though with some more thinking I see how this isn't extremely useful, tbh. wraitii: This is also not for testing, but used for e.g. sending work from the pathfinder, so we can… | |||||||||||||||
private: | |||||||||||||||
class Impl; | |||||||||||||||
std::unique_ptr<Impl> m; | |||||||||||||||
}; | |||||||||||||||
} // namespace ThreadPool | |||||||||||||||
Not Done Inline Actionsnamespace. vladislavbelov: `namespace`. | |||||||||||||||
#endif // INCLUDED_THREADPOOL | |||||||||||||||
Not Done Inline Actionsconst? Stan: const? | |||||||||||||||
Done Inline Actions(on this and above) wraitii: (on this and above)
I think it's technically const, but semantically it doesn't make much sense… | |||||||||||||||
Not Done Inline Actionsconst? Stan: const? | |||||||||||||||
Not Done Inline ActionsUse static method instead. vladislavbelov: Use static method instead. | |||||||||||||||
Not Done Inline ActionsIt seems the Pool name is overused. Pool can't do any work it only can provide threads and not thread execution interfaces. For that purposes there is TaskManager or TaskQueue. vladislavbelov: It seems the `Pool` name is overused. Pool can't do any work it only can provide threads and… |