unix,win: limit concurrent DNS calls to nthreads/2 #1845

addaleax · 2018-05-25T20:48:30Z

Not sure about semverness here, so I went with PRing against master first. Happy to retarget to v1.x if that’s deemed okay as well. Also happy to throw this PR in the garbage bin if this approach doesn’t work for people.

If nthreads / 2 (rounded up) DNS calls are outstanding,
queue more work of that kind instead of letting it take over
more positions in the thread pool, blocking other work
such as the (usually much faster) file system I/O or
user-scheduled work.

Fixes: nodejs/node#8436

addaleax · 2018-05-25T20:50:25Z

Oh, also: Not sure how to write a race-condition-free test for this.

gireeshpunathil · 2018-05-26T02:29:14Z

LGTM

bnoordhuis

Left some comments but I'm okay with the general idea.

bnoordhuis · 2018-05-26T08:20:13Z

include/uv/threadpool.h

  void (*done)(struct uv__work *w, int status);
  struct uv_loop_s* loop;
  void* wq[2];
+  enum uv__work_kind kind;


This is an ABI change and therefore semver-major. You probably don't want to wait for that to show up in a release because libuv 2.0 is still some time out.

You should be able to drop this field if you move the slow_io_pooled_work >= (nthreads + 1) / 2 logic to worker(), have that function check both queues, and have post() simply put it in the right queue based on kind (and move the enum the src/uv-common.h, please.)

bnoordhuis · 2018-05-26T08:22:18Z

src/threadpool.c

+static void post(QUEUE* q, enum uv__work_kind kind) {
  uv_mutex_lock(&mutex);
+  if (kind == UV__WORK_SLOW_IO) {
+    if (slow_io_pooled_work >= (nthreads + 1) / 2) {


Looks like this could result in starvation (perennial preemption by fast work reqs) when nthreads == 1.

gireeshpunathil · 2018-05-26T13:26:15Z

I would like to test the case of nodejs/node#8436 with this change, but looks difficult - as the current Node.js uses v1.x and this change is on top of master, and the gap seems to be wide. Any advice please, or else I will attempt to hand-pick the changes into the libuv present in the latest Node. thanks.

gireeshpunathil · 2018-05-29T08:11:51Z

never mind the previous comment, I manually applied this change onto the latest node and tested the reproduce in nodejs/node#8436, and the result is wonderful!

without the change

with the change:

The first number indicates time gap between file I/O, and the second indicates the number of concurrent dns I/O that is being processed. The result clearly shows that the file I/O is immune to the dns look up.

If `nthreads / 2` (rounded up) DNS calls are outstanding, queue more work of that kind instead of letting it take over more positions in the thread pool, blocking other work such as the (usually much faster) file system I/O or user-scheduled work. Fixes: nodejs/node#8436

addaleax · 2018-06-09T20:01:55Z

@gireeshpunathil @bnoordhuis Updated the PR for 1.x with no public ABI changes, along the lines of Ben’s suggestion. Could you take another look?

I’ve tried to avoid adding even more complexity here; As a result, this does have a slightly different scheduling behaviour (multiple “slow” tasks always start executing a full queue length apart from each other). I think that’s okay, though.

gireeshpunathil · 2018-06-11T12:33:10Z

LGTM

bnoordhuis

LGTM, CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/931/

bnoordhuis · 2018-06-14T21:59:42Z

src/threadpool.c

 static void worker(void* arg) {
  struct uv__work* w;
  QUEUE* q;
+  int is_slow_work;


The only proper name for this variable is is_slooow_work.

^{I'll get my coat.}

This is too tempting :)

bnoordhuis · 2018-06-14T22:13:31Z

src/threadpool.c

+    QUEUE_REMOVE(q);
+    QUEUE_INIT(q);  /* Signal uv_cancel() that the work req is executing. */
+
+    if (q == &run_slow_work_message) {


For legibility, I'd is_slow_work = 0; before the if and drop the else. It's below the fold, so to speak; easy to miss.

bnoordhuis · 2018-06-14T22:18:14Z

src/threadpool.c

+static QUEUE run_slow_work_message;
+static QUEUE slow_io_pending_wq;

+#define SLOW_WORK_THREAD_THRESHOLD ((nthreads + 1) / 2)


Maybe make this a function. The fact that it's a macro hides that it's not a constant but depends on a global.

bnoordhuis

Still LGTM but this should probably get sign-off from at least one more maintainer since it's a pretty subtle change.

bnoordhuis · 2018-06-28T14:21:18Z

@libuv/collaborators Ping. Can at least one additional maintainer give this a quick lookover?

santigimeno

LGTM with a couple of questions

santigimeno · 2018-06-28T16:47:13Z

src/uv-common.h


+enum uv__work_kind {
+  UV__WORK_CPU,
+  UV__WORK_FAST_IO,


What's the difference between UV__WORK_CPU and UV__WORK_FAST_IO?

Nothing, at this point

santigimeno · 2018-06-28T16:56:00Z

src/threadpool.c

+        QUEUE_INSERT_TAIL(&wq, &run_slow_work_message);
+        if (idle_threads > 0)
+          uv_cond_signal(&cond);
+      }


Does this mean if we have a scenario with works scheduled in this order:

slow_work_1
slow_work_2
fast work_1
fast_work_2
fast_work_3

the slow_work_2 would be scheduled after the fast works even though the threshold for slow works was 2? If so, was it intentional?

It was born out of necessity – an earlier version of this patch treated things differently, but as far as I can tell it’s hard to do this another way without changing the uv_work_t layout to include a uv__work_kind field

santigimeno · 2018-06-29T08:41:41Z

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/955/

addaleax · 2018-07-13T17:25:30Z

Hm, gentle ping? CI results are inaccessible by now, so I guess this needs a new one … :)

gireeshpunathil · 2018-08-14T13:22:32Z

requesting this to be taken forward as there is no outstanding concerns.

santigimeno · 2018-08-14T14:37:37Z

Sorry for the delay, I was waiting for someone else to comment and forgot about it.
CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/990/

One comment:
@addaleax, would it make sense, later on, having a different PR for master with the uv__work_kind field in uv_work_t as you had originally?

santigimeno · 2018-08-18T12:28:09Z

CI libuv-in-node: https://ci.nodejs.org/view/libuv/job/libuv-in-node/53/

addaleax · 2018-08-18T17:45:21Z

@santigimeno

would it make sense, later on, having a different PR for master with the uv__work_kind field in uv_work_t as you had originally?

I think so, yes. It’s not important, though.

If `nthreads / 2` (rounded up) DNS calls are outstanding, queue more work of that kind instead of letting it take over more positions in the thread pool, blocking other work such as the (usually much faster) file system I/O or user-scheduled work. Fixes: nodejs/node#8436 PR-URL: #1845 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]>

santigimeno · 2018-08-21T08:52:34Z

Landed in 90891b4. Thanks!

This commit implements the "pluggable threadpool" design. This commit reverts libuv#1845 because it encodes a policy decision. This decision is up to the embedders now. The implementation is Linux-only at the moment. Still have to change: - win/fs.c - win/getaddrinfo.c - win/getnameinfo.c

90891b4 introduced a race condition when accessing `slow_io_work_running` – it is being increased and later decreased as part of the worker thread loop, but was accessed with different mutexes during these operations. This fixes the race condition by making sure both accesses are protected through the global `mutex` of `threadpool.c`. This fixes a number of flaky Node.js tests. Refs: libuv#1845 Refs: nodejs/reliability#18 Refs: nodejs/node#23089 Refs: nodejs/node#23067 Refs: nodejs/node#23066 Refs: nodejs/node#23219

90891b4232e91dbd7a2e2077e4d23d16a374b41d introduced a race condition when accessing `slow_io_work_running` – it is being increased and later decreased as part of the worker thread loop, but was accessed with different mutexes during these operations. This fixes the race condition by making sure both accesses are protected through the global `mutex` of `threadpool.c`. This fixes a number of flaky Node.js tests. Refs: libuv/libuv#1845 Refs: nodejs/reliability#18 Refs: nodejs#23089 Refs: nodejs#23067 Refs: nodejs#23066 Refs: nodejs#23219

90891b4 introduced a race condition when accessing `slow_io_work_running` – it is being increased and later decreased as part of the worker thread loop, but was accessed with different mutexes during these operations. This fixes the race condition by making sure both accesses are protected through the global `mutex` of `threadpool.c`. This fixes a number of flaky Node.js tests. Refs: #1845 Refs: nodejs/reliability#18 Refs: nodejs/node#23089 Refs: nodejs/node#23067 Refs: nodejs/node#23066 Refs: nodejs/node#23219 PR-URL: #2021 Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

Validate that massive dns lookups do not block filesytem I/O (or any fast I/O for that matter). Prior to libuv/libuv#1845 few back-to-back dns lookup were sufficient to engage libuv threadpool workers in a blocking manner, throttling other work items that need the pool. this test acts as a regression test for the same. Start slow and fast I/Os together, and make sure fast I/O can complete in at least in 1/100th of time for slow I/O. Refs: libuv/libuv#1845 Refs: nodejs#8436 PR-URL: nodejs#23099 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Validate that massive dns lookups do not block filesytem I/O (or any fast I/O for that matter). Prior to libuv/libuv#1845 few back-to-back dns lookup were sufficient to engage libuv threadpool workers in a blocking manner, throttling other work items that need the pool. this test acts as a regression test for the same. Start slow and fast I/Os together, and make sure fast I/O can complete in at least in 1/100th of time for slow I/O. Refs: libuv/libuv#1845 Refs: #8436 PR-URL: #23099 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Validate that massive dns lookups do not block filesytem I/O (or any fast I/O for that matter). Prior to libuv/libuv#1845 few back-to-back dns lookup were sufficient to engage libuv threadpool workers in a blocking manner, throttling other work items that need the pool. this test acts as a regression test for the same. Start slow and fast I/Os together, and make sure fast I/O can complete in at least in 1/100th of time for slow I/O. Refs: libuv/libuv#1845 Refs: nodejs#8436 PR-URL: nodejs#23099 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

This commit implements the "pluggable threadpool" design. This commit reverts libuv#1845 because it encodes a policy decision. This decision is up to the embedders now. The implementation is Linux-only at the moment. Still have to change: - win/fs.c - win/getaddrinfo.c - win/getnameinfo.c

Validate that massive dns lookups do not block filesytem I/O (or any fast I/O for that matter). Prior to libuv/libuv#1845 few back-to-back dns lookup were sufficient to engage libuv threadpool workers in a blocking manner, throttling other work items that need the pool. this test acts as a regression test for the same. Start slow and fast I/Os together, and make sure fast I/O can complete in at least in 1/100th of time for slow I/O. Refs: libuv/libuv#1845 Refs: #8436 PR-URL: #23099 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

addaleax mentioned this pull request May 25, 2018

worker: initial implementation nodejs/node#20876

Closed

4 tasks

bnoordhuis reviewed May 26, 2018

View reviewed changes

addaleax force-pushed the slow-fast-thread branch from 930213f to 0724df2 Compare June 9, 2018 19:33

addaleax changed the base branch from master to v1.x June 9, 2018 19:33

addaleax force-pushed the slow-fast-thread branch 2 times, most recently from 42e0989 to 16a9fe6 Compare June 9, 2018 19:51

[squash] ABI compatibility

aa5506a

addaleax force-pushed the slow-fast-thread branch from 16a9fe6 to aa5506a Compare June 9, 2018 19:53

bnoordhuis approved these changes Jun 14, 2018

View reviewed changes

addaleax added 2 commits June 15, 2018 13:20

[squash] review comments

080bace

[squash] undo now-unrelated whitespace change

0cc4693

bnoordhuis approved these changes Jun 18, 2018

View reviewed changes

santigimeno approved these changes Jun 28, 2018

View reviewed changes

santigimeno closed this Aug 21, 2018

refack mentioned this pull request Aug 21, 2018

dns.lookup blocks filesystem I/O nodejs/node#8436

Closed

This was referenced Aug 27, 2018

Threadpool meta-issue #1959

Closed

WIP: threadpool: pluggable threadpool #1726

Closed

addaleax deleted the slow-fast-thread branch August 27, 2018 21:02

davisjam mentioned this pull request Aug 29, 2018

Libuv thread pool UV_THREADPOOL_SIZE in a node cluser setup nodejs/node#22468

Closed

davisjam mentioned this pull request Sep 14, 2018

worker_threads or the threadpool? nodejs/help#1494

Closed

gireeshpunathil mentioned this pull request Sep 26, 2018

test: add uv threadpool congestion regression test nodejs/node#23099

Merged

4 tasks

addaleax mentioned this pull request Oct 4, 2018

unix,win: fix threadpool race condition #2021

Closed

davisjam mentioned this pull request Oct 12, 2018

unref support for dns requests #203

Closed

gireeshpunathil mentioned this pull request Dec 26, 2019

make crypto functions execute in the threadpool asynchronously nodejs/node#678

Closed

GiovanniMancini mentioned this pull request Dec 9, 2020

[#176030906] Add option to use dns.resolve on http clients pagopa/io-ts-commons#340

Closed

unix,win: limit concurrent DNS calls to nthreads/2 #1845

unix,win: limit concurrent DNS calls to nthreads/2 #1845

Uh oh!

Conversation

addaleax commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented May 25, 2018

Uh oh!

gireeshpunathil commented May 26, 2018

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gireeshpunathil commented May 26, 2018

Uh oh!

gireeshpunathil commented May 29, 2018

Uh oh!

addaleax commented Jun 9, 2018

Uh oh!

gireeshpunathil commented Jun 11, 2018

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Jun 28, 2018

Uh oh!

santigimeno left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

santigimeno commented Jun 29, 2018

Uh oh!

addaleax commented Jul 13, 2018

Uh oh!

gireeshpunathil commented Aug 14, 2018

Uh oh!

santigimeno commented Aug 14, 2018

Uh oh!

santigimeno commented Aug 18, 2018

Uh oh!

addaleax commented Aug 18, 2018

Uh oh!

santigimeno commented Aug 21, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

addaleax commented May 25, 2018 •

edited

Loading