Done in next diff.

Done in next diff. Same for others. :)

Sorry, I don't understand what you mean here. You're saying we should implement a "Thread Manager" class at a 'top-level' global that tells us how many threads we want for pathfinding?
I'm not clear how that's much better for now.

Also I somewhat intend to remove this as if we had proper mutex protection it wouldn't be needed.

well, in this part of code threads are going to be created, they do not exists at this point so no concurency here.

wantedThreads is local (in scope of function) variable - > no problem even with threads as every would have its own unique variable with no option to see or change the one from another thread

g_Atlas & & g_Atlas->running is only reading, as far as I know you can safely read from variable with as many threads as you wish and it is safe but again this function is running in main thread only

Sorry, I don't understand what you mean here. You're saying we should implement a "Thread Manager" class at a 'top-level' global that tells us how many threads we want for pathfinding?

I mean that the component shouldn't decide about the number of threads and shouldn't communicate with the Atlas directly. We may use ThreadUtil to calculate right number of threads. Because we might have separate threads for other purposes, and we need to manage them somehow.

g_Atlas & & g_Atlas->running is only reading, as far as I know you can safely read from variable with as many threads as you wish and it is safe but again this function is running in main thread only

It's not the syntax error but the semantic for me.

OK, makes sense for ThreadUtil.

64 is max value in UI, and so should be there. Why not 255?
This case is intended to protect players from setting invalid values. values <0 are handled like 0 is, and I've assumed that user probably wouldn't want to run more than 64 threads. I don't know anyone who has 32-core CPU. We can however raise this limit a bit. 255 seems reasonable value.

Indeed.

Might as well return.

Fine, didn't know about that.

It didn't work on linux until very recently thanks to @linkmauve

You can't use a condition_variable alone to wait on multiple threads. Adding the proper synchronisation would require more code, which has a higher chance of mistakes now and down the line.
I agree that this spins at 100% which is a bit of a waste. What I hope is that using std::this_thread::yield() fixes this issue with an acceptable latency.

Yes, in a direct way, but you can wrap it. Like https://stackoverflow.com/questions/24465533/implementing-boostbarrier-in-c11/24465624#24465624. Or use something like boost::barrier.

Adding the proper synchronisation would require more code, which has a higher chance of mistakes now and down the line.

It's not much more code and it's relatively simple. I'd prefer user's comfort instead of not saving few lines of code. In fact you may freeze the user machine (if it's not so good to predict busy-waiting). Also it may slow down other threads, I mean like renderer one, if it'd exist.

std::this_thread::yield

It's useful but it doesn't guarantee a good stable work. It may wake up every tick or sleep for a long time.

Could you measure how many percents it and threads do busy waiting?

I've ran some benchmarks:

• Yield() isn't useful on OS X, and when spin looping hard we are consuming 100% CPU which seems wasteful.

• condition variables and barriers seem almost faster than spinlocking.

Overall I'll switch to using condition_variable. I think it's a complicated construct, but at the end of the day we can't occupy 100% of a process spin locking.

It's just there to be specialised, and it pushes requests so I don't think so?

That line doesn't pushes request does it ?

It inserts them at the end (by moving), so that's similar to pushing.

static_assert(sizeof(T) == 0, "Only specializations can be used"); What part of that statement does that ? Oo There is only one line in that function

As said above, this is just the template function which gets specialised by L709/714. This base template cannot be used (it's not implemented but there's no clean way to write that because of issues in XCode/VS, see https://stackoverflow.com/a/37593094).

lock gets release automatically at the end of the scope.

It's trickier, but it's also a micro-improvement that needs further testing.

It's hard enough. There's a high chance of dumb bugs, and this whole diff is already stupidly faster than svn.

This is another spin-lock: we continue looping until all threads are done computing, so it needs to be reset at the beginning of the loop anyways. Putting it outside the loop isn't much neater imo, but the whole thing might be rewritten in a slightly more functional way I think.

One can't actually, because I need to call either the longRequest or the shortRequest of the worker in the inner loop. This is basically rewritable as

CCmpPathfinder::PushRequestsToWorkers(std::vector<T>& from, bool toLongRequests)
[...]
if (toLongRequests)
	worker->m_LongRequests.insert([...])
else
	worker->m_ShortRequests.insert([...])

If I pass the worker, I lose the whole point of the function as the for-loop has to be outside the function.

If you wanna leave the function loop, I'd prefer to add more code but with an explicit access. Like:

template<typename T>
CCmpPathfinder::PushRequestsToWorkers(std::vector<T>& from)
{
    // ...
    worker->AddRequests<T>(
        std::make_move_iterator(from.end() - amount),
        std::make_move_iterator(from.end())
    );
    // ...
}

template<typename T>
void AsyncPathfinderWorkerThread::AddRequests(... it_begin, ... it_end)
{
    auto& container = RequestsContainer<T>();
    container.insert(container.begin(), std::make_move_iterator(it_begin), std::make_move_iterator(it_end));
}

template<typename T>
void AsyncPathfinderWorkerThread::RequestsContainer();
template<>
std::deque<AsyncShortPathRequest>& RequestsContainer() { return m_ShortRequests; }
// ...

It'd be the same by performance, but safer I think. As you can't pass an arbitrary container member.

I suppose ultimately this would be if-constexpr-ed. I'll go with your way as it does seem more readable and I didn't think of it.

Done in a slightly simpler manner.

sure :)

Well, not exaclty 'for new turn', because StartProcessingRequests is called three times during single turn.
I must check whether it's just remaining of non-threaded version, or if it's still needed...
Anyway, it's not gonna happen unless there's a bug.

mutex spin-lock