[ThreadPool] Run a thread per CPU in SimpleExecutor #104
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ChuanqiXu9
changed the title
RainMark
reviewed
May 16, 2022
async_simple/util/ThreadPool.h
Outdated
| // Run threads per core. | ||
| cpu_set_t cpuset; | ||
| CPU_ZERO(&cpuset); | ||
| CPU_SET(i, &cpuset); |
There was a problem hiding this comment.
1、cpu id maybe not start at 0 or not be sequenced in container environment
2、_threadNum maybe great than available cpus
3、macOS support pthread？
There was a problem hiding this comment.
- According to https://linux.die.net/man/3/cpu_set, the cpu id starts at 0 and is continuous.
- I refactored it to
CPU_SET(i % CPU_SETSIZE, &cpuset);So it wouldn't overflow. - I didn't find related things in https://developer.apple.com/search/?q=CPU_SET. Given the users who would need performance in macOS should be less, I think we could give up to support them.
RainMark
reviewed
May 16, 2022
| int rc = pthread_setaffinity_np(_threads[i].native_handle(), | ||
| sizeof(cpu_set_t), &cpuset); | ||
| if (rc != 0) | ||
| std::cerr << "Error calling pthread_setaffinity_np: " << rc << "\n"; |
There was a problem hiding this comment.
we need a log component？
There was a problem hiding this comment.
Yeah, I filed an issue at #105
|
Do you compare the performance using this improvement? @ChuanqiXu9 |
ChuanqiXu9
force-pushed
the
main
branch
2 times, most recently
from
05dbd9b
to
0fa280a
Compare
Yeah, I found this one when looking at #99 |
|
LGTM |
RainMark
reviewed
May 17, 2022
| cpu_set_t cpuset; | ||
| CPU_ZERO(&cpuset); | ||
| CPU_SET(cpu_ids[i % cpu_ids.size()], &cpuset); | ||
| int rc = pthread_setaffinity_np(_threads[i].native_handle(), |
There was a problem hiding this comment.
we can use sched_setaffinity syscall to avoid include pthread，because already be linux system， but it is also ok to use pthread
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
Now we don't bind threads in ThreadPool to physical CPU cores. So we might not be able to get the expected concurrency.
What is changing
This one should be transparent for users.