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

Add OS.get_physical_processor_count() method #76932

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented May 11, 2023

This returns the number of physical CPU cores on the system, as opposed to OS.get_processor_count() which returns the number of logical CPU cores on the system.

Use cases:

  • Configure parallelism for algorithms that don't scale well to logical CPU cores. In most cases, using the number of logical cores as a basis is recommended, but there are select cases where using a lower core count that matches the physical core count is more suited.
  • Troubleshoot performance issues, as you can now detect whether a user has HyperThreading enabled or not (by checking whether OS.get_physical_processor_count() and OS.get_processor_count() are different).

Testing project: test_physical_cpu_count.zip

TODO

  • Fix Windows implementation.
  • Test macOS implementation. I tested this on a M1 Mac mini and it returns 8 which sounds expected.

@Calinou Calinou force-pushed the os-add-get-physical-processor-count branch 3 times, most recently from 028c664 to 0f50c62 Compare August 9, 2023 08:08
@@ -150,6 +150,20 @@ String OS_LinuxBSD::get_unique_id() const {
return machine_id;
}

int OS_LinuxBSD::get_physical_processor_count() const {
Ref<FileAccess> f = FileAccess::open("/proc/cpuinfo", FileAccess::READ);
Copy link
Member

Choose a reason for hiding this comment

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

This probably won't work on BSD, so something sysctl based, similar to the macOS, probably should be added.

It seems to be kern.smp.cores for FreeBSD, not sure about other BSDs (maybe hw.ncpu).

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added code to support *BSD but I can't test it, since I currently don't have a VM setup active.

@Calinou Calinou force-pushed the os-add-get-physical-processor-count branch from 0f50c62 to 34df9eb Compare January 10, 2024 22:31
@Calinou Calinou force-pushed the os-add-get-physical-processor-count branch from 34df9eb to e2982fd Compare October 22, 2024 20:20
This returns the number of physical CPU cores on the system, as opposed
to `OS.get_processor_count()` which returns the number of logical CPU cores
on the system.

Use cases:

- Configure parallelism for algorithms that don't scale well to logical
  CPU cores.
- Troubleshoot performance issues, as you can now detect whether a user
  has HyperThreading enabled or not
  (by checking whether `OS.get_physical_processor_count()` and
  `OS.get_processor_count()` are not equal).
@Calinou Calinou force-pushed the os-add-get-physical-processor-count branch from e2982fd to 13a3e43 Compare November 19, 2024 22:10
int OS_MacOS::get_physical_processor_count() const {
uint32_t num_cores = 0;
size_t num_cores_len = sizeof(num_cores);
sysctlbyname("hw.physicalcpu", &num_cores, &num_cores_len, 0, 0);
Copy link
Member

@Ivorforce Ivorforce Dec 18, 2024

Choose a reason for hiding this comment

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

sysctlbyname may error, returning != 0:

https://developer.apple.com/documentation/kernel/1387446-sysctlbyname#return_value

I don't know though if it actually does in any practical situation.

@darksylinc
Copy link
Contributor

I feel the documentation should mention the result value should be cached because calling get_physical_processor_count() can potentially be expensive.

@bruvzg
Copy link
Member

bruvzg commented Dec 18, 2024

I feel the documentation should mention the result value should be cached because calling get_physical_processor_count() can potentially be expensive.

Any reasons not to cache it internally? Processor count should not change in runtime.

@darksylinc
Copy link
Contributor

Any reasons not to cache it internally? Processor count should not change in runtime.

I was thinking the exact same thing as I wrote it. Only thing to watch out is race conditions when caching the first time.

It is possible for complex computers to have CPUs turned off (and maybe become mainstream in the future?), but if that's becomes a reality for the consumer, caching it externally or caching it internally won't matter (plus, a whole lot of other problems more than just this function).

So yeah, no reason we cannot cache it internally.

@hpvb
Copy link
Member

hpvb commented Dec 18, 2024

For people using virtual machines it could be useful to get the actual current CPU count. But that's a bit of a niche case.

I'm not entirely convinced that this is entirely useful though, Godot has no way to provide any kind of thread affinity, so getting the physical core count really isn't information you can do much with. Just running your heavy process on half the number of cores doesn't really necessarily prevent sharing on smt logical cores.

I don't really care about the method, but if we are going to document it as being good for that we need a whole pile of other infrastructure to actually tell the OS that we want tasks split away from core siblings.

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.

7 participants