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

Improve speed for windows Get-CimInstance #1053

Merged
merged 1 commit into from
Jun 8, 2024

Conversation

Earlopain
Copy link
Contributor

This is a small followup for #1051.

In terms of the previous code, the powershell invocation currently does select * from Win32_Processor which is slower. I missed this in the other PR, this improves the speed when detection runs on windows.

The pipe is still needed since that command returns all properties anyways, they are just empty/not computed.

This only fetches a single property which is faster
Copy link
Collaborator

@eregon eregon left a comment

Choose a reason for hiding this comment

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

Thanks.
Do you have any measurements before & after?

@eregon eregon merged commit 4ea1fc5 into ruby-concurrency:master Jun 8, 2024
14 checks passed
@Earlopain
Copy link
Contributor Author

Earlopain commented Jun 8, 2024

Yeah, sure. My bad, I should have lead with that:

On my machine the old command takes about 1s, this new one only about 0.2s.

I've measured this inside powershell by wrapping the expressions inside Measure-Command { ... } which I guess is its equivalent to time.

@Earlopain Earlopain deleted the speedup-for-win-cpu branch June 8, 2024 18:42
@eregon
Copy link
Collaborator

eregon commented Jun 8, 2024

Could you also measure wmic cpu get NumberOfCores? I wonder how fast that is.

Yeah 1s seems a lot, I'll try to do a release with this improvement soon.

@Earlopain
Copy link
Contributor Author

Earlopain commented Jun 8, 2024

wmic completes in about 0.1s. Some overhead comes from starting up powershell, that alone takes about 50ms.win32ole is a c extension and (probably) doesn't go through the trouble of starting a process in the first place: it completes in ~60ms

Unfortunately the wmic command is deprecated, https://learn.microsoft.com/en-us/windows/win32/wmisdk/wmic and the powershell command is the recommended alternative, even if a bit slower.

@eregon
Copy link
Collaborator

eregon commented Jun 9, 2024

I wonder, is powershell available on all Windows machines?
Otherwise we could have an issue if/when wmic is removed, if it's possible to also not have powershell on these Windows machines.

@eregon
Copy link
Collaborator

eregon commented Jun 9, 2024

FWIW I left a comment on https://bugs.ruby-lang.org/issues/20309#note-22, making win32ole a bundled gem seems to cause more harm than help (in performance & portability).

@Earlopain
Copy link
Contributor Author

Well, the speed regression is my fault, I just wasn't careful enough. As for if powershell will be available, I would say you can rely on this as much as cmd.exe. Sometime in Windows 10/11 they actually made it the default shell.

The features available of this gem go far beyond just querying wmi, like interacting with excel/word, or other modern software like internet explorer. Basically a full-on automation interface. If you look at the gem itself, the C code to pipe that all through to ruby is quite extensive.

@Earlopain
Copy link
Contributor Author

In that sense it was more like using a canon to kill a fly. It is sad that etc doesn't have a equivalent for this since nprocessors exists

@eregon
Copy link
Collaborator

eregon commented Jun 9, 2024

To clarify I didn't mean to criticize what you did, the opposite, it is much appreciated :)
I am happy I didn't need to figure this out myself (and I don't have a Windows Ruby setup).
I'll make a release with this optimization.

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.

2 participants