classification
Title: ThreadPoolExecutor doesn't reuse threads until #threads == max_workers
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Matt Spitz, bquinlan, dmacnet, inada.naoki, iunknwn, josh.r, pitrou, tomMoral, torsten
Priority: normal Keywords: patch

Created on 2015-08-17 14:59 by Matt Spitz, last changed 2018-04-24 22:32 by iunknwn.

Files
File name Uploaded Description Edit
many_threads.py torsten, 2016-05-30 20:09
Pull Requests
URL Status Linked Edit
PR 6375 open python-dev, 2018-04-04 21:38
PR 6530 open iunknwn, 2018-04-19 01:59
Messages (17)
msg248732 - (view) Author: Matt Spitz (Matt Spitz) Date: 2015-08-17 14:59
https://hg.python.org/cpython/file/3.4/Lib/concurrent/futures/thread.py#l114

ThreadPoolExecutor will keep spawning new threads, even if existing threads are waiting for new work. We should check against the queue length when deciding to spawn a new thread to avoid creating unnecessary threads.
msg248749 - (view) Author: Matt Spitz (Matt Spitz) Date: 2015-08-17 20:09
On further investigation, it appears that we can't just check against the queue length, as it doesn't indicate whether threads are doing work or idle.

A change here will need a counter/semaphore to keep track of the number of idle/working threads, which may have negative performance implications.
msg266716 - (view) Author: Torsten Landschoff (torsten) * Date: 2016-05-30 20:09
For demonstration purposes, here is a small example specifically for Linux which shows how each request starts a new thread even though the client blocks for each result.
msg266772 - (view) Author: Josh Rosenberg (josh.r) * Date: 2016-05-31 19:58
Is there a good reason to worry about overeager worker spawning? ProcessPoolExecutor spawns all workers when the first work item is submitted ( https://hg.python.org/cpython/file/3.4/Lib/concurrent/futures/process.py#l361 ), only ThreadPoolExecutor even makes an effort to limit the number of threads spawned. Threads are typically more lightweight than processes, and with the recent GIL improvements, the CPython specific costs associated with threads (particularly threads that are just sitting around waiting on a lock) are fairly minimal.

It just seems like if eager process spawning isn't a problem, neither is (cheaper) eager thread spawning.
msg279144 - (view) Author: David MacKenzie (dmacnet) Date: 2016-10-21 17:54
If each worker thread ties up other resources in an application, such as handles to server connections, conserving threads could have a significant impact. That's the situation for an application I am involved with.

I've written and tested a patch to make this change, using a second Queue for the worker threads to notify the executor in the main thread by sending a None when they finish a WorkItem and are therefore idle and ready for more work. It's a fairly simple patch. It does add a little more overhead to executing a job, inevitably. I can submit the patch if there's interest. Otherwise, perhaps the TODO comment in thread.py should be rewritten to explain why it's not worth doing.
msg279145 - (view) Author: David MacKenzie (dmacnet) Date: 2016-10-21 18:10
This issue seems to overlap with 14119.
msg315007 - (view) Author: iunknwn (iunknwn) * Date: 2018-04-05 22:45
I've submitted a PR that should resolve this - it uses a simple atomic counter to ensure new threads are not created if existing threads are idle. 

One concern I do have - while writing the patch, I noticed the existing submit method (specifically the adjust_thread_count function) isn't thread safe. I've added more details in the PR.
msg315090 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-04-08 18:32
I'm not fond of this proposal.  The existing behaviour is harmless; especially for a thread pool, since threads are cheap resources.  Improving the logic a bit might seem nice, but it also complicates the executor implementation a bit more.

Besides, once the N threads are spawned, they remain alive until the executor is shut down. So all it takes is a spike in incoming requests and you don't save resources anymore.
msg315091 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-04-08 18:33
> If each worker thread ties up other resources in an application, such as handles to server connections, conserving threads could have a significant impact.

You may want to implement a pooling mechanism for those connections, independent of the thread pool.  It is also probably more flexible (you can implement whichever caching and lifetime logic benefits your application).
msg315092 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-04-08 18:42
Side note:

> One concern I do have - while writing the patch, I noticed the existing submit method (specifically the adjust_thread_count function) isn't thread safe.

True.  The executor is obviously thread-safe internally (as it handles multiple worker threads).  But the user should not /call/ it from multiple threads.

(most primitives exposed by the Python stdlib are not thread-safe, except for the simplest ones such as lists, dicts etc.)
msg315260 - (view) Author: iunknwn (iunknwn) * Date: 2018-04-13 19:36
The existing behavior seems strange (and isn't well documented). The code had a TODO comment from bquinlan to implement idle thread recycling, so that was why I made the change. 

That said, if threads are cheap, why not just create all the work threads on initialization, and then remove all the logic entirely?

Also, regarding the executor and thread-safety, there's an example in the current docs showing a job being added to the executor from a worker thread (it's part of the example on deadlocks, but it focuses on max worker count, not on the executor's thread-safety).
msg315261 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-04-13 20:04
> That said, if threads are cheap, why not just create all the work threads on initialization, and then remove all the logic entirely?

That would sound reasonable to me.  bquinlan has been absent for a long time, so I wouldn't expect an answer from him on this issue.

> Also, regarding the executor and thread-safety, there's an example in the current docs showing a job being added to the executor from a worker thread

Actually, looking at the code again, submit() is protected by the shutdown_lock, so it seems it should be thread-safe.  That's on git master btw.
msg315264 - (view) Author: iunknwn (iunknwn) * Date: 2018-04-13 20:18
Alright - I'll put together another patch that removes the logic, and spins up all threads during initialization. 

Do you want me to create a completely new PR, or just update my existing one?
msg315265 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-04-13 20:23
Creating a new PR would be cleaner IMHO.
msg315471 - (view) Author: iunknwn (iunknwn) * Date: 2018-04-19 02:01
Done - as recommend, I've opened a new PR that changes the behavior to spawn all worker threads when the executor is created. This eliminates all the thread logic from the submit function.
msg315485 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) Date: 2018-04-19 11:04
Why not just remove TODO comment?
Thread is cheap, but not zero-cost.
msg315715 - (view) Author: iunknwn (iunknwn) * Date: 2018-04-24 22:32
I feel like there are two reasonable options here:

1) We can implement a thread pool with basic resource tracking. This means idle threads get recycled, and threads that have been sitting idle for a while are terminated as demand drops, so resources can be reclaimed. This wouldn't require very much work - we would just need to modify some of the queue operations to have timeouts, and track the number of idle threads (most of that's already in my first PR). We could easily add options like min_threads and idle_thread_timeout as options in kwargs to the init routine. 

2) We can skip all tracking, and spin a fixed number of threads at initialization. This removes the complexity of locks and counts, and means the thread pool executor will work identically to the process pool executor (which also eagerly spawns resources). If we want this, this is ready to go in the second PR. 

I personally like option 1 because it feels closer to other languages I've worked in, but I'd like a bit more guidance from the reviewers before proceeding.
History
Date User Action Args
2018-04-24 22:32:23iunknwnsetmessages: + msg315715
2018-04-19 11:04:02inada.naokisetnosy: + inada.naoki
messages: + msg315485
2018-04-19 02:01:36iunknwnsetmessages: + msg315471
2018-04-19 01:59:20iunknwnsetpull_requests: + pull_request6224
2018-04-13 20:23:48pitrousetmessages: + msg315265
2018-04-13 20:18:33iunknwnsetmessages: + msg315264
2018-04-13 20:04:38pitrousetmessages: + msg315261
2018-04-13 19:36:55iunknwnsetmessages: + msg315260
2018-04-08 18:42:56pitrousetmessages: + msg315092
2018-04-08 18:33:07pitrousetmessages: + msg315091
2018-04-08 18:32:20pitrousetnosy: + tomMoral

messages: + msg315090
versions: + Python 3.8
2018-04-08 18:25:06ned.deilysetnosy: + bquinlan, pitrou
2018-04-05 22:45:50iunknwnsetmessages: + msg315007
components: + Library (Lib)
2018-04-05 22:30:25iunknwnsetnosy: + iunknwn
2018-04-04 21:38:00python-devsetkeywords: + patch
stage: patch review
pull_requests: + pull_request6087
2016-10-21 18:10:06dmacnetsetmessages: + msg279145
2016-10-21 17:54:16dmacnetsetnosy: + dmacnet
messages: + msg279144
2016-05-31 19:58:31josh.rsetnosy: + josh.r
messages: + msg266772
2016-05-30 20:09:36torstensetfiles: + many_threads.py
nosy: + torsten
messages: + msg266716

2015-08-17 20:09:44Matt Spitzsetmessages: + msg248749
2015-08-17 20:03:25Matt Spitzsettitle: ThreadPoolExceutor doesn't reuse threads until #threads == max_workers -> ThreadPoolExecutor doesn't reuse threads until #threads == max_workers
2015-08-17 14:59:00Matt Spitzcreate