Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Elastic threadpool RFC #382

Closed
wants to merge 4 commits into from
Closed

Conversation

bnoordhuis
Copy link
Member

Something I worked on over the weekend and would like to get some feedback on. The premise is that threads are started on demand and scale up to some TBD limit. I'll comment inline on places that still need improvement.

One benefit is that it makes it possible to use libuv in an environment that restricts threading.

#include <stdlib.h>

#define MAX_THREADPOOL_SIZE 128
#define IDLE_THREAD_TIMEOUT 5e9 /* 5 seconds in nanoseconds. */
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arbitrary timeout. Maybe this should be configurable with an option for 'never'.

@saghul
Copy link
Member

saghul commented Jun 2, 2015

I'll have a look!

@bnoordhuis
Copy link
Member Author

@saghul WDYT?

@saghul
Copy link
Member

saghul commented Jun 16, 2015

Sorry, got distracted :-S I'll review tomorrow for sure.

QUEUE_INSERT_TAIL(&wq, q);
uv_cond_signal(&cond);
uv_mutex_unlock(&mutex);
static int new_detached_thread(void) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should uv_thread_self work if called from a worker thread? We use a TLS key for that, which wouldn't be setup in this case.

Maybe we can use uv_thread_create and fiddle with the internals here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize the TLS was used for that. There is one reason (well, two) why I implemented it like this:

  1. uv_thread_create() on Windows creates the thread in suspended mode, then calls ResumeThread(). I don't quite understand why it does that so I decided not to mess with it.
  2. uv_thread_create() calls malloc() and free() from different threads, which might be slow. For example, glibc has to acquire a process-global lock when the free happens on a different thread.

We can just try it and see how well it works.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on Windows creates the thread in suspended mode, then calls ResumeThread(). I don't quite understand why it does that so I decided not to mess with it.

It does it so it can store the thread handle as the current thread in TLS.

We can just try it and see how well it works.

Sounds reasonable.

@saghul
Copy link
Member

saghul commented Jun 17, 2015

Left some comments, but the idea SGTM. Some pseudo-random thoughts on the matter:

  • should we have a min_threads concept as well? That is, we can scale elastically, but always keep a set of threads available, to avoid the overhead of always having to create one if operations in the threadpool are sporadic.
  • since the threadpool would now be elastic, why have a max threads clamp?
  • how about some uv_threadpool_resize API to change the min and max threads at runtime?

@bnoordhuis
Copy link
Member Author

should we have a min_threads concept as well? That is, we can scale elastically, but always keep a set of threads
how about some uv_threadpool_resize API to change the min and max threads at runtime?

That sounds reasonable.

since the threadpool would now be elastic, why have a max threads clamp?

To stop runaway thread creation? Most operating systems can only handle a limited number of threads before performance and throughput (both in-process and system-wide) starts to seriously degrade.

An unlimited number of threads also isn't that useful if the individual threads are mostly CPU-bound (which they normally aren't with internal work but can be for work that comes in through uv_queue_work().)

@saghul
Copy link
Member

saghul commented Jun 17, 2015

To stop runaway thread creation? Most operating systems can only handle a limited number of threads before performance and throughput (both in-process and system-wide) starts to seriously degrade.

Just to clarify, I mean this clamp:

#define MAX_THREADPOOL_SIZE 128

If the user wants to set the maximum to 256 and bring their systems to a halt it's all good by me :-) I'd go with a note in the documentation.

@saghul
Copy link
Member

saghul commented Sep 19, 2016

Closing this in favor of #267. @bnoordhuis will be writing a new LEP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants