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

Windows: Implement cpu_physical_cores and cpu_cores #145

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Carterpersall
Copy link

Hello, I'm a contributor to https://github.com/lptstr/winfetch (you've talked to the maintainer in #112), and would like to start applying some of the experience I gained there to this project.

@CarterLi
Copy link

You may get physical cores and logical cores ( threads ) at the same time with GetLogicalProcessorInformationEx(RelationAll)

https://github.com/LinusDierheimer/fastfetch/blob/b3da6b0e89c0decb9ea648e1d98a75fa6ac40225/src/detection/cpu/cpu_windows.c#L17

Because this method requires copying more data from the kernel, I'm not sure whether it's faster than your implementation. Just another idea.

@Carterpersall
Copy link
Author

Carterpersall commented Mar 27, 2023

I added several alternative implementations

Implementation Speeds:

  • Command used: hyperfine --warmup 20 --prepare "cmd" --min-runs 200 ".\macchina.exe --show processor"
    • Compiled in release mode

Plugged in:

  • GetLogicalProcessorInformation
    • 13.6 ms
  • Environment Variable
    • 13.7 ms
  • Registry
    • 13.7 ms
  • GetNativeSystemInfo
    • 14.0 ms
  • Physical Cores (also GetLogicalProcessorInformation)
    • 14.0 ms

On Battery life:

  • GetLogicalProcessorInformation
    • 30.8 ms
  • Environment Variable
    • 31.2 ms
  • Registry
    • 31.1 ms
  • GetNativeSystemInfo
    • 30.7 ms
  • Physical Cores (also GetLogicalProcessorInformation)
    • 31.1 ms

When heavily throttled (used power plans to downclock CPU to 1.06 GHz to simulate a slow computer)

  • GetLogicalProcessorInformation
    • 55 ms
  • Environment Variable
    • 53.5 ms
  • Registry
    • 54.6 ms
  • GetNativeSystemInfo
    • 54.3 ms
  • Physical Cores (also GetLogicalProcessorInformation)
    • 54.7 ms

@grtcdr
Copy link
Member

grtcdr commented Mar 27, 2023

There's virtually no difference between some of the implementations and those of num_cpus, why not call those two functions directly?

@CarterLi
Copy link

CarterLi commented Mar 28, 2023

According to stackoverflow, WMI, registry and GetLogicalProcessorInformationEx are only reliable ways to get the number of cpu logical cores (threads)

@Carterpersall
Copy link
Author

Carterpersall commented Apr 12, 2023

According to stackoverflow, WMI, registry and GetLogicalProcessorInformationEx are only reliable ways to get the number of cpu logical cores (threads)

That does seem to be the case. I've made a commit that addresses it. cpu_cores now uses GetLogicalProcessorInformationEx to get the number of cores, and has Registry and WMI based implementations as backups. cpu_physical_cores now uses GetLogicalProcessorInformationEx as well.

I used the implementation here as the basis of my implementation of GetLogicalProcessorInformationEx.

@IAMSolaara
Copy link

Hi, I'm making a tool that can benefit from this.
Any way we can get this merged?

@grtcdr
Copy link
Member

grtcdr commented Sep 1, 2024

I've got a Windows VM lying around but I'm a bit busy at the moment. If this works correctly, I don't have a problem accepting the PR. I would appreciate @uttarayan21's input on the state of this PR because he's much more knowledgeable on Windows than I am.

@@ -295,11 +297,159 @@ impl GeneralReadout for WindowsGeneralReadout {
}

fn cpu_physical_cores(&self) -> Result<usize, ReadoutError> {
Err(ReadoutError::NotImplemented)
// Source: https://github.com/AFLplusplus/LibAFL/blob/main/libafl/src/bolts/core_affinity.rs#L423
Copy link

Choose a reason for hiding this comment

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

404

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants