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

processor, v3/processor (Win): fix slow cpuinfo on multisocket config #1088

Merged
merged 1 commit into from
Jun 19, 2021

Conversation

fredwangwang
Copy link

updated win32_Processor struct to exclude (unused) LoadPercentage field.
The loadpercentage takes linearly more time as the # of sockets
increases. By default vSphere maps 1 vCPU to 1 socket, resulting in very
poor performance when getting CPU info against, saying, 40 vCPU VM
(basically 40 sockets as seen by the VM).

Here is the before and after time comparison:
printcpu-diff

changes:

for cpu:
since Win32_Processor is a public struct, created a substruct named: Win32_ProcessorWithoutLoadPct for use in all queries without breaking any potential usage of the public api.

for v3/cpu:
since the struct now is private, simply removed LoadPercentage

updated win32_Processor struct to exclude loadpercentage field.
The loadpercentage takes linearly more time as the # of sockets
increases. By default vSphere maps 1 vCPU to 1 socket, resulting in very
poor performance when getting CPU info against, saying, 40 vCPU VM
(basically 40 sockets as seen by the VM).
@idrennanvmware
Copy link

idrennanvmware commented Jun 17, 2021

@shirou - any chance we could prioritize this? This particular issue is trickling down through our usage of Nomad and in turn the long fingerprint times on our ESXI hosts are causing issues. Very much appreciate the work you've done, and our ability to contribute to this. If you need us to address anything on the PR please let us know and we will be happy to do so.

Thanks!

@shirou
Copy link
Owner

shirou commented Jun 17, 2021

I can only have a time on weekends.

@shirou
Copy link
Owner

shirou commented Jun 19, 2021

Thank you for your contribution and your patiant.

  • As you mentioned, LoadPercentage is not used currently. so please just remove LoadPercentage without creating another struct?
  • Could you also implement to v3 in same way?

@fredwangwang
Copy link
Author

fredwangwang commented Jun 19, 2021

Hi @shirou thanks for reviewing!

See my comments above:

changes:

for cpu:
since Win32_Processor is a public struct, created a substruct named: Win32_ProcessorWithoutLoadPct for use in all queries without breaking any potential usage of the public api.

for v3/cpu:
since the struct now is private, simply removed LoadPercentage

However if you are fine with "breaking" the contract for cpu.Win32_Processor, I am more than happy to update the pr and simply remove LodaPercentage as well, pls let me know 🙂

@shirou
Copy link
Owner

shirou commented Jun 19, 2021

Oh, OK, I understand. Then, it is a good implementation! Thank you so much!

@fredwangwang
Copy link
Author

Hi @shirou looks like the shellcheck is the only part fails, but that feels unrelated to the pr's change. Could you help to take a look at that? Thank you!

@shirou
Copy link
Owner

shirou commented Jun 19, 2021

some of the tests are stoled but mostly ok. So I merge this PR. Thank you!

@shirou shirou merged commit fcbe555 into shirou:master Jun 19, 2021
@idrennanvmware
Copy link

Thanks so much @fredwangwang and @shirou !

@fredwangwang
Copy link
Author

Hi @shirou is it possible to cut a new release? The downstream package consumer (hashicorp nomad) prefers a tagged build instead of using a specific sha.

Thanks in advance!

@shirou
Copy link
Owner

shirou commented Jun 22, 2021

gopsutil will be released on end of month.

@fredwangwang
Copy link
Author

fredwangwang commented Jun 22, 2021 via email

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.

None yet

3 participants