Skip to content

single writer threadpool #1565

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

Merged
merged 4 commits into from
Feb 6, 2021
Merged

single writer threadpool #1565

merged 4 commits into from
Feb 6, 2021

Conversation

swilly22
Copy link
Contributor

@swilly22 swilly22 commented Feb 4, 2021

No description provided.

@swilly22 swilly22 requested a review from DvirDukhan February 4, 2021 10:15
@swilly22 swilly22 self-assigned this Feb 4, 2021
DvirDukhan
DvirDukhan previously approved these changes Feb 4, 2021
Comment on lines 68 to 71
tid = thpool_num_threads(_thpool);
} else {
// +1 to compensate for Redis main thread
tid = thpool_get_thread_id(_thpool, pthread_self()) + 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIU
for a pool with size 1
the reader thread will get tid=1 which is also thpool_num_threads
is it a problem here?

jeffreylovitz
jeffreylovitz previously approved these changes Feb 4, 2021
@@ -13,14 +13,24 @@
#include "../util/rmalloc.h"
#include "../util/thpool/thpool.h"

extern threadpool _readers_thpool; // declared in module.c
extern threadpool _writers_thpool; // declared in module.c

static int get_thread_id() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add explanatory comment like:

/* get_thread_id returns 0 for the Redis main thread,
  * reader thread count + 1 for the writer thread,
  * or reader thread ID for a reader thread. */

We should consider relocating this function to thpool.c, since it gets invoked here and by command_ctx.c (though thankfully nowhere else).

@swilly22 swilly22 dismissed stale reviews from jeffreylovitz and DvirDukhan via 40e2891 February 6, 2021 10:09
@swilly22 swilly22 force-pushed the single-worker-threadpool branch from 20a865b to 2ec0eb2 Compare February 6, 2021 12:22
@swilly22 swilly22 force-pushed the single-worker-threadpool branch from 09a0612 to 176eacd Compare February 6, 2021 12:59
@swilly22 swilly22 merged commit 01b22e1 into master Feb 6, 2021
@swilly22 swilly22 deleted the single-worker-threadpool branch February 6, 2021 12:59
swilly22 added a commit that referenced this pull request Feb 8, 2021
* single writer threadpool

* name read and writer threads

* maintain both readers and writers thread pools under a single pools object
pnxguide pushed a commit to CMU-SPEED/RedisGraph that referenced this pull request Mar 22, 2023
* single writer threadpool

* name read and writer threads

* maintain both readers and writers thread pools under a single pools object
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants