misc: introduce `uv_set_threadpool_size` #4415

juanarbol · 2024-05-21T22:36:21Z

This call will set the threadpool size from libuv and set UV_THREADPOOL_SIZE environment variable.

This is my approach for #4401; and of course I'm open to changes and suggestions.
I decided to mutate UV_THREADPOOL_SIZE considering that maybe some Node.js users or anyone relies on that variable for certain situations (like monitoring or something). And this is not a thread safe call as uv_os_setenv is not. Even tho, I don't is possible make this thread safe on any way.

cjihrig · 2024-05-21T23:02:37Z

uv_set_threadpool_size() seems like a more intuitive name IMO.

juanarbol · 2024-05-21T23:03:37Z

uv_set_threadpool_size() seems like a more intuitive name IMO.

I think you are right. Will rename and fix the windows under ASAN. It seems to be related with #4338

saghul · 2024-05-22T05:54:01Z

docs/src/misc.rst

+ This function must be called before any other libuv function is called.
+
+ On success, it returns 0, if the threadpool is already spining it returns
+ `UV_EINVAL`.


Maybe UV_EALREADY or UV_EBUSY are better error codes?

saghul · 2024-05-22T05:56:21Z

src/threadpool.c

+ char num_str[12]; /* enough room for unsigned int and null terminator */
+
+ /* check if the threadpool is already initialized */
+ if (nthreads != 0)


I wonder: would it be possible to enlarge the amount of threads while the pool is running?

Reducing it is probably hard / impossible, but adding new threads might be within the realm of possibility.

I think that is out of the scope of the issue itself. I could work on that as well. It could be a mix between @santigimeno suggestion of a getter for this and this PR rn.

I have a branch from 2015 that does just that: https://github.com/bnoordhuis/libuv/commits/auto-threadpool

Shrinking is done lazily. Threads terminate when they're idle and the actual thread count > desired thread count.

@bnoordhuis Do you think I should pivot this approach into something like yours instead?

I'd say that's a good idea. #382 is the PR from back then, you will want to read through the discussion.

cc @saghul as a fyi. You had some comments back then; maybe you still do?

joy4eg · 2024-05-22T10:24:51Z

docs/src/misc.rst

@@ -886,3 +886,15 @@ is not complete.
 for the NUL terminator.

 .. versionadded:: 1.47.0
+
+.. c:function:: int uv_set_threadpool_size(unsingned int number)


Should we use size_t ?

Makes sense

Nevermind, the code seems to like unsigned int instead.

Your spin_threads function takes size_t

santigimeno · 2024-05-22T10:39:22Z

include/uv.h

+UV_EXTERN int uv_set_threadpool_size(unsigned int number);
+


Adding a uv_get_threadpool_size() would be a good idea, I think.

That could be useful as well

src/threadpool.c

juanarbol · 2024-05-22T15:34:00Z

Thanks for the review, everyone, after reading all the comments and suggestions.

I will extend the functionalities over the setter, if UV_THREADPOOL_SIZE is not provided, will spin as many threads as requested in this call. If env is provided; that will be rule and there is nothing users can do to spin more threads or a different number of threads.
If called again with more threads, spins k more threads
Introduce the getter.

juanarbol · 2024-05-23T00:15:24Z

The new revision includes the getter and the behaviour is a bit different now as this patch surpasses the functionalities of the original issue. And most importantly, it respects UV_THREADPOOL_SIZE as the first source of truth and won't mutate it.

What a painful and joyful adventure 🫠

juanarbol · 2024-05-23T00:23:45Z

I'll see what is wrong with Windows later, maybe tomorrow, but feel free to review anyway. </3

juanarbol · 2024-05-23T04:29:18Z

r = uv_os_setenv("UV_THREADPOOL_SIZE", "1");
ASSERT_OK(r);
#ifdef _WIN32
  /* juanarbol: why? */
  ASSERT_EQ(nthreads, 4);
#else
  /* The default thread pool size is UV_THREADPOOL_SIZE */
  ASSERT_EQ(nthreads, 1);
#endif

This is odd. If anyone can confirm if windows simply does not work or have any idea about this, would be really appreciated.

src/threadpool.c

saghul · 2024-05-23T05:16:19Z

src/threadpool.c

@@ -206,7 +225,7 @@ static void init_threads(void) {
 nthreads = MAX_THREADPOOL_SIZE;

 threads = default_threads;
- if (nthreads > ARRAY_SIZE(default_threads)) {
+ if (nthreads != ARRAY_SIZE(default_threads)) {


Why allocate memory if we need fewer threads than the default?

Yeah. Right, that was from a different experiment

src/threadpool.c

saghul · 2024-05-23T05:21:23Z

docs/src/misc.rst

@@ -886,3 +886,15 @@ is not complete.
 for the NUL terminator.

 .. versionadded:: 1.47.0
+
+.. c:function:: int uv_set_threadpool_size(unsingned int number)


Your spin_threads function takes size_t

include/uv.h

joy4eg · 2024-05-23T06:12:37Z

r = uv_os_setenv("UV_THREADPOOL_SIZE", "1");
ASSERT_OK(r);
#ifdef _WIN32
  /* juanarbol: why? */
  ASSERT_EQ(nthreads, 4);
#else
  /* The default thread pool size is UV_THREADPOOL_SIZE */
  ASSERT_EQ(nthreads, 1);
#endif
This is odd. If anyone can confirm if windows simply does not work or have any idea about this, would be really appreciated.

setenv() just sets the env, nothing more. 4 is the default thread pool size, and that's why you got it.

juanarbol

Thanks! I will address all the comments and wait for your opinions if we need all those locks

juanarbol · 2024-05-23T06:31:45Z

```c
r = uv_os_setenv("UV_THREADPOOL_SIZE", "1");
ASSERT_OK(r);
#ifdef _WIN32
  /* juanarbol: why? */
  ASSERT_EQ(nthreads, 4);
#else
  /* The default thread pool size is UV_THREADPOOL_SIZE */
  ASSERT_EQ(nthreads, 1);
#endif
This is odd. If anyone can confirm if windows simply does not work or have any idea about this, would be really appreciated.

setenv() just sets the env, nothing more. 4 is the default thread pool size, and that's why you got it.

I was expecting that env var to affect the threadpool size, like it does in XNIX

saghul · 2024-05-23T07:40:46Z

src/threadpool.c

-
- uv_sem_destroy(&sem);
+ /* Spin up the default number of threads */
+ spin_threads(0 /* from */, nthreads /* to */);


This call is fine because it's inside init_threads, which is guarateed to only run once. Any other place where this is called needs to hold onto the mutex.

src/threadpool.c

saghul · 2024-05-23T07:45:14Z

test/test-threadpool-size.c

+ RETURN_SKIP("Test does not currently work on Windows under ASAN");
+#endif
+
+ r = uv_os_setenv("UV_THREADPOOL_SIZE", "1");


Not sure how reliable this is, since any chance in the test runner what uses any fs API will trigger a threadpool initialization.

Is not each test -at least in this case- like a "isolated" event loop?

juanarbol · 2024-05-23T18:25:21Z

Ok, after addressing all the comments here, I think this is thread safe now :)

Thanks for reviewing, explanations and your time. Of course I'm open for more changes if needed.

juanarbol · 2024-06-24T19:43:12Z

Humble ping @saghul, the CI failure is unrelated to this. And about the environment test case, I think it is worth having it, that describes the whole feature behaviour. But I can remove it if needed.

This patch will allow to grow and set the thread pool size on demand via libuv calls instead of just `UV_THREADPOOL_SIZE` but `UV_THREADPOOL_SIZE` will remain as the initial value. Fixes: libuv#4401 Signed-off-by: Juan José Arboleda <[email protected]>

juanarbol force-pushed the juan/threadpool branch 2 times, most recently from af05da2 to e13cf0c Compare May 21, 2024 22:51

juanarbol force-pushed the juan/threadpool branch 2 times, most recently from 0217b12 to 2b0aa45 Compare May 21, 2024 23:14

juanarbol changed the title ~~misc: introduce uv_threadpool_set_number~~ misc: introduce uv_set_threadpool_size May 21, 2024

juanarbol force-pushed the juan/threadpool branch from 2b0aa45 to 1d15545 Compare May 21, 2024 23:28

saghul reviewed May 22, 2024

View reviewed changes

joy4eg reviewed May 22, 2024

View reviewed changes

santigimeno requested changes May 22, 2024

View reviewed changes

juanarbol force-pushed the juan/threadpool branch from 1d15545 to dc41425 Compare May 23, 2024 00:11

juanarbol force-pushed the juan/threadpool branch from dc41425 to 5dde4b0 Compare May 23, 2024 04:17

saghul reviewed May 23, 2024

View reviewed changes

juanarbol commented May 23, 2024

View reviewed changes

saghul requested changes May 23, 2024

View reviewed changes

juanarbol force-pushed the juan/threadpool branch from 5dde4b0 to 1e5aae6 Compare May 23, 2024 18:21

juanarbol force-pushed the juan/threadpool branch from 1e5aae6 to bd74ca1 Compare May 23, 2024 18:35

juanarbol force-pushed the juan/threadpool branch 2 times, most recently from b007828 to c85be07 Compare June 5, 2024 20:10

juanarbol force-pushed the juan/threadpool branch from c85be07 to 9bad1d8 Compare June 24, 2024 19:33

juanarbol force-pushed the juan/threadpool branch from 9bad1d8 to 34e566d Compare July 3, 2024 03:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc: introduce `uv_set_threadpool_size` #4415

misc: introduce `uv_set_threadpool_size` #4415

juanarbol commented May 21, 2024

cjihrig commented May 21, 2024

juanarbol commented May 21, 2024

saghul May 22, 2024

juanarbol May 22, 2024

saghul May 22, 2024

juanarbol May 22, 2024 •

edited

Loading

bnoordhuis Jun 20, 2024

juanarbol Jul 3, 2024

bnoordhuis Jul 11, 2024

joy4eg May 22, 2024

juanarbol May 22, 2024

juanarbol May 22, 2024

saghul May 23, 2024

santigimeno May 22, 2024

juanarbol May 22, 2024

juanarbol commented May 22, 2024

juanarbol commented May 23, 2024 •

edited

Loading

juanarbol commented May 23, 2024

juanarbol commented May 23, 2024

saghul May 23, 2024

juanarbol May 23, 2024

saghul May 23, 2024

joy4eg commented May 23, 2024

juanarbol left a comment

juanarbol commented May 23, 2024

saghul May 23, 2024

saghul May 23, 2024

juanarbol May 23, 2024

juanarbol commented May 23, 2024

juanarbol commented Jun 24, 2024

misc: introduce uv_set_threadpool_size #4415

Are you sure you want to change the base?

misc: introduce uv_set_threadpool_size #4415

Conversation

juanarbol commented May 21, 2024

cjihrig commented May 21, 2024

juanarbol commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanarbol May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanarbol commented May 22, 2024

juanarbol commented May 23, 2024 • edited Loading

juanarbol commented May 23, 2024

juanarbol commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joy4eg commented May 23, 2024

juanarbol left a comment

Choose a reason for hiding this comment

juanarbol commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanarbol commented May 23, 2024

juanarbol commented Jun 24, 2024

misc: introduce `uv_set_threadpool_size` #4415

misc: introduce `uv_set_threadpool_size` #4415

juanarbol May 22, 2024 •

edited

Loading

juanarbol commented May 23, 2024 •

edited

Loading