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

std thread hardware_concurrency() should not be used #130

Closed
jbd opened this issue Feb 2, 2023 · 3 comments
Closed

std thread hardware_concurrency() should not be used #130

jbd opened this issue Feb 2, 2023 · 3 comments

Comments

@jbd
Copy link

jbd commented Feb 2, 2023

Hello,

std::thread::hardware_concurrency() returns, when possible, the underlying hardware capability to run threads, which might not corresponds to the actual number of cores available to the process (through the use of taskset, batch system like slurm, etc...). The consequence is that mkdwarfs might run in a non optimal way. For example, if I run taskset -c 1 mkdwarfs on my 20 cores machines, it will run 20 workers on only one core.

The immediate workaround is to use the -N option to set the number of workers, but I think a more sane behavior would be to use sched_getaffinity as in opencv/opencv#16268. Gromacs did something similar (https://github.com/gromacs/gromacs/blob/1e6873fadf16d5f5be861e6f9ef5f9923a12e540/src/gromacs/hardware/hardwaretopology.cpp#L1221).

What do you think ?

Thank you.

@jbd
Copy link
Author

jbd commented Feb 2, 2023

In fact, the solution from folly/folly/system/HardwareConcurrency.cpp is great !

#include <folly/system/HardwareConcurrency.h>

#include <thread>

#include <folly/portability/Sched.h>

namespace folly {

unsigned int hardware_concurrency() noexcept {
#if defined(__linux__) && !defined(__ANDROID__)
  cpu_set_t cpuset;
  if (!sched_getaffinity(0, sizeof(cpuset), &cpuset)) {
    auto count = CPU_COUNT(&cpuset);
    if (count != 0) {
      return count;
    }
  }
#endif

  return std::thread::hardware_concurrency();
}

} // namespace folly

@mhx
Copy link
Owner

mhx commented Feb 3, 2023

Hey, thanks for the report! Switching to the folly implementation shouldn't be too much trouble. I'll consider that for the next release.

mhx added a commit that referenced this issue Mar 7, 2023
@mhx
Copy link
Owner

mhx commented Mar 7, 2023

Fixed in e4971b4.

@mhx mhx closed this as completed Mar 7, 2023
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

No branches or pull requests

2 participants