`<execution>`: Should threadpool callback priority be influenced by parent thread priority?

[StephanTLavavej]

Our C++17 parallel algorithms use the Windows threadpool:

STL/stl/inc/execution

Line 213 in 165fc94

: _Ptp_work(::__std_create_threadpool_work(&_Work::_Threadpool_callback, _STD addressof(_Operation), nullptr)) {

STL/stl/src/parallel_algorithms.cpp

Lines 23 to 26 in 165fc94

	[[nodiscard]] PTP_WORK __stdcall __std_create_threadpool_work(
	PTP_WORK_CALLBACK _Callback, void* _Context, PTP_CALLBACK_ENVIRON _Callback_environ) noexcept {
	return CreateThreadpoolWork(_Callback, _Context, _Callback_environ);
	}

We're passing null to CreateThreadpoolWork for the callback environment:

If this parameter is NULL, the callback executes in the default callback environment. For more information, see InitializeThreadpoolEnvironment.

Therefore, the threadpool callbacks have default priority, regardless of the parent thread priority.

Think-cell reported this to us, raising the concern of priority inversion.

In theory, we could alter our code to set up a callback environment, and use SetThreadpoolCallbackPriority to request low, normal, or high priority. However, it is unclear to me whether "The priority for the callback relative to other callbacks in the same thread pool." would control the priority relative to the parent thread, which is what would be needed to address priority inversion concerns.

Ultimately, the conclusion may be that thread priority is outside the realm of the Standard, so if an application has used the Windows API to alter its thread priorities, then the C++17 parallel algorithms are a poor fit for it.

[AlexGuteniev]

My opinion is wontfix.

Below is justification of this opinion.

---

SetThreadPoolCallbackPriority has too few choices comparing to usual thread priorities.

I also don't think that these priorities even map anyhow to usual thread priorities.

Thread pools are expected to handle gracefully short tasks, so that internal calls to SetThreadPriority performed by thread pool threads would be too much perf waste.

So I expect that thread pool doesn't call SetThreadPriority, and the priorities set via SetThreadPoolCallbackPriority is user mode thing that has no relationship in OS thread priority. And the originating thread priority is irrelevant.

As a hypothetically possible fix I only see that each thread would maintain its own thead pool with thread pool thread priorities matching the original thead priority. WinAPI thead pool doesn't allow setting its threads priority, at least not in a documented safe way, so would have to maintain own thread pool implementation. Also there's no way to track changes of master thread priority. In short, that's not seems feasible in practice.

So to me it looks like that an application that relies on non-default thread priorities has to avoid the use of thread pool in non-default prioritized threads. This includes avoidance of parallel algorithms, non-deferred std::async, #pragma omp, or /Qpar , whatever.

[wanghan02]

In our tests, setting the thread priority in the callback to the thread priority of the parent thread, and restoring it on exit of the callback works, does not have a big overhead, and makes a significant difference in the performance of UI thread running simultaneously. Doing it once per range item is obviously inefficient, but once per callback is probably fine and potentially helps a lot.

[AlexGuteniev]

Doing it once per range item is obviously inefficient, but once per callback is probably fine and potentially helps a lot

This semi-defeats the purpose of parallel algorithms by adding heavy (though constant time) operation to each callback, and making scheduling callbacks less efficient. Though if you won't do it for normal priority parent thread, the normal case won't be penalized, so it may be fine.

(I wouldn't want even GetThreadPriority() of parent thread penalty, but that's not my call)

[schoedl]

Many interactive applications, which is arguably the majority of applications on Windows, will use thread priorities to make the UI thread more responsive. Actually, the majority of CPU-heavy work will likely be done in the background. Effectively excluding all of them from using the C++ standard library seems a bad idea. The difference is massive: for us the UI thread got 3x faster by lowering the background thread priorities in the for_each body.

[AlexGuteniev]

Many interactive applications, which is arguably the majority of applications on Windows, will use thread priorities to make the UI thread more responsive

This doesn't sound as anything common or widespread. I never encountered such a recommendation, or seen the code of an UI application that actually does this.

[schoedl]

Well, maybe they should because it makes the UI thread 3x faster. There is nothing special about what we are doing, besides both UI and background being CPU-bound. I have no reason to believe other applications would not get a similar result.

[StephanTLavavej]

We talked about this at the weekly maintainer meeting and we don't believe that any changes should be made here.

Having threadpool callbacks manually adjust the priority of threadpool worker threads (which the STL has not directly created, and could be reused by other components) would "contaminate" the thread pool, which seems undesirable. Even if the callback attempts to restore the threadpool worker thread's priority, because the STL isn't directly controlling how callbacks are executed in the threadpool, the chance for unwanted interactions seems high.

It sounds to us like C++17 parallel algorithms are a poor fit for your scenario (because of the Standard's lack of recognition of thread priority), and/or other designs should be investigated - e.g. a UI thread probably shouldn't start a parallel algorithm and block on its completion, instead that scenario sounds like it's suited for promises/futures, so that the UI thread can periodically check whether the long-running operation is complete, and display a placeholder/spinner/etc. while the operation is running.

[AlexGuteniev]

There is nothing special about what we are doing, besides both UI and background being CPU-bound

Well, that sounds special. To make the UI responsive, you avoid making UI thread CPU-bound. From my experiences, this has always been feasible, even when showing quasi-relatime 60 fps chart. I acknowledge that this may be not feasible for your case, but this is definitely not a problem encountered by most of application, not something usual.

[wanghan02]

Having threadpool callbacks manually adjust the priority of threadpool worker threads (which the STL has not directly created, and could be reused by other components) would "contaminate" the thread pool, which seems undesirable. Even if the callback attempts to restore the threadpool worker thread's priority, because the STL isn't directly controlling how callbacks are executed in the threadpool, the chance for unwanted interactions seems high.

In Thread Pools.Best Practices.3, it says user should restore thread priority before returning from the call back function. If modifying thread priority in the call back would "contaminate" the thread pool, it should be forbidden instead of restoring the original thread priority as in the best practices.

Clean up all resources created in the callback function before returning from the function. These include TLS, security contexts, thread priority, and COM registration. Callback functions must also restore the thread state before returning.

The problem we are facing is that, parallel algorithms from c++17 should be able to handle CPU-bound tasks gracefully. If we have a low priority background thread doing some CPU-bound work and it calls a parallel algorithm, this parallel algorithm should NOT simply dispatch the work loads onto THREAD_PRIORITY_NORMAL threads. The current implementation with Thread Pool does so which makes those supposed-to-be low priority work loads from low priority background thread

• win the competition against other low priority threads which greatly delays other low priority threads, and
• compete against other normal priority threads which makes those normal priority threads much slower.

I can see that modifying the thread priority in each call back function is not the most efficient thing and the impact of the modification is not clear or even well documented. But throwing everything onto the threads (which have THREAD_PRIORITY_NORMAL) of the Thread Pool is simply wrong. I don't think c++17 parallel algorithms' usage should be limited to being called by only normal priority threads in the real world. If Thread Pool cannot handle it gracefully we need another implementation. As a workaround, we found that modifying thread priority in the call back function work as expected even though it's not perfect.