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

Use logical CPUs instead of physical by default #2391

Merged
merged 1 commit into from
Apr 9, 2020
Merged

Conversation

seanmonstar
Copy link
Member

@seanmonstar seanmonstar commented Apr 9, 2020

Some reasons to prefer logical count as the default:

  • Chips reporting many logical CPUs vs physical, such as via
    hyperthreading, probably know better than us about the workload the CPUs
    can handle.
  • The logical count (num_cpus::get()) takes into consideration
    schedular affinity, and cgroups CPU quota, in case the user wants to
    limit the amount of CPUs a process can use.

Closes #2269

Some reasons to prefer logical count as the default:

- Chips reporting many logical CPUs vs physical, such as via
hyperthreading, probably know better than us about the workload the CPUs
can handle.
- The logical count (`num_cpus::get()`) takes into consideration
schedular affinity, and cgroups CPU quota, in case the user wants to
limit the amount of CPUs a process can use.
@jebrosen
Copy link
Contributor

jebrosen commented Apr 9, 2020

Looks like this also closes #2269.

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

lgtm! freebsd build is just busted.

@seanmonstar seanmonstar merged commit d294c99 into master Apr 9, 2020
@seanmonstar seanmonstar deleted the num-cpus-get branch April 9, 2020 19:42
hawkw added a commit that referenced this pull request Apr 9, 2020
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Apr 9, 2020
# 0.2.17 (April 9, 2020)

### Fixes
- rt: bug in work-stealing queue (#2387) 

### Changes 
- rt: threadpool uses logical CPU count instead of physical by default
  (#2391)


Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@jonhoo
Copy link
Sponsor Contributor

jonhoo commented Apr 9, 2020

This is fantastic! So many hacks to set the thread pool size when I want to run with fewer cores can go away 🎉

@Vagelis-Prokopiou
Copy link

@seanmonstar @jonhoo I had to look into this matter too, in the context of actix-web.

In my specific case, the configuration with workers = number of physical cpus outperformed the configuration with workers = number of logical cpus. You can check this issue for specifics: actix/actix-web#957

I believe that some amount of benchmarks should be the judge in this matter, because there is a possibility that the performance is degraded out of the box with this configuration.

@jonhoo
Copy link
Sponsor Contributor

jonhoo commented Dec 18, 2021

@Vagelis-Prokopiou I suspect that's because hyperthreading is generally bad for performance (at least in my experience). It's a tricky trade-off. What I'd really like for us to do is to respect cgroups/affinity, but ignore hyperthreading, but I don't know that that's feasible to implement 😅

@Vagelis-Prokopiou
Copy link

Vagelis-Prokopiou commented Dec 19, 2021

@jonhoo I am not that aware of the internals in order to provide a good proposal.

What I am proposing is:

a) Some benchmarks by various people, in order to decide the best default option from the currently available options.

b) If option a is not possible, at least better documentation for the current setup (I created a PR for this #4330).

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.

Default number of threads seem to be equal to num physical cores, not logical cores
6 participants