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

Twitter coverage #928

Open
ovinusreal opened this issue Aug 18, 2024 · 6 comments
Open

Twitter coverage #928

ovinusreal opened this issue Aug 18, 2024 · 6 comments

Comments

@ovinusreal
Copy link

Recently, the package was criticized on Twitter as a dependency of the Discord native app for unnecessarily many invocations of Powershell. I'm quoting them here in case they are valid feedback:

it's using a library called systeminformation that calls powershell like "Get-WmiObject Win32_logicaldisk | select Access,Caption,FileSystem,FreeSpace,Size ${drive ? '| where -property Caption -eq ' + drive : ''} | fl" instead of doing anything properly.

const cmd = `Get-WmiObject Win32_logicaldisk | select Access,Caption,FileSystem,FreeSpace,Size ${drive ? '| where -property Caption -eq ' + drive : ''} | fl`;

they're executing from DriverStore, assuming System32 AND hardcoding C:\Windows

[...]

this code scans EIGHT HUNDRED different folders to try and find an nvidia-smi binary.

function getNvidiaSmi() {
if (_nvidiaSmiPath) {
return _nvidiaSmiPath;
}
if (_windows) {
try {
const basePath = util.WINDIR + '\\System32\\DriverStore\\FileRepository';
// find all directories that have an nvidia-smi.exe file
const candidateDirs = fs.readdirSync(basePath).filter(dir => {
return fs.readdirSync([basePath, dir].join('/')).includes('nvidia-smi.exe');
});
// use the directory with the most recently created nvidia-smi.exe file
const targetDir = candidateDirs.reduce((prevDir, currentDir) => {
const previousNvidiaSmi = fs.statSync([basePath, prevDir, 'nvidia-smi.exe'].join('/'));
const currentNvidiaSmi = fs.statSync([basePath, currentDir, 'nvidia-smi.exe'].join('/'));
return (previousNvidiaSmi.ctimeMs > currentNvidiaSmi.ctimeMs) ? prevDir : currentDir;
});
if (targetDir) {
_nvidiaSmiPath = [basePath, targetDir, 'nvidia-smi.exe'].join('/');
}
} catch (e) {
util.noop();
}
} else if (_linux) {
_nvidiaSmiPath = 'nvidia-smi';
}
return _nvidiaSmiPath;
}

const WINDIR = process.env.WINDIR || 'C:\\Windows';

The former appears to be already addressed in #927

They also recommend using GetACP instead of having to assume codepage 437.

function getCodepage() {
if (_windows) {
if (!codepage) {
try {
const stdout = execSync('chcp', execOptsWin);
const lines = stdout.toString().split('\r\n');
const parts = lines[0].split(':');
codepage = parts.length > 1 ? parts[1].replace('.', '').trim() : '';
} catch (err) {
codepage = '437';
}
}
return codepage;
}
if (_linux || _darwin || _freebsd || _openbsd || _netbsd) {
if (!codepage) {
try {
const stdout = execSync('echo $LANG', util.execOptsLinux);
const lines = stdout.toString().split('\r\n');
const parts = lines[0].split('.');
codepage = parts.length > 1 ? parts[1].trim() : '';
if (!codepage) {
codepage = 'UTF-8';
}
} catch (err) {
codepage = 'UTF-8';
}
}
return codepage;
}
}

I am not familiar with Windows development, and some of these don't provide better alternatives, so feel free to close this issue if the claims were baseless or made without context.

Nonetheless, thanks for the work you've put into this project!

@sylveon
Copy link

sylveon commented Aug 19, 2024

Most of these WMI usages can be replaced by native Win32 APIs giving the same info (for example, to get the free space on a drive, GetDiskFreeSpace).

Otherwise, if WMI is really needed, there is a native WMI API that can be used instead of forking out to PowerShell (which is really slow). There is the node-wmi module exposing this API to JavaScript. (nevermind that just calls to wmic which is deprecated)

@sebhildebrandt
Copy link
Owner

@ovinusreal thank you for pointing me to this discussion. I know (and this is also stated in the docs) that on Windows the situation is far from being optimal. This is mostly related to the absence of easy ways to gather system relevant information in Windows.

The first route I took was using WMI (same approach as we have in the node-mwi package that @sylveon mentioned. Biut WMI is deprecated so the only way is to use PowerShell to get information via command line (all other OS do provide such functionality much easier!).

I am working on a new version of this library that should make the PowerShell calls more efficient but It will still take some time to finish this new version (6.0) as there are also a lot of new functions that I am planning to finish.

My take way is still some improvements mentioned in the twitter chat that I will try to come up with a better solution.

@sylveon
Copy link

sylveon commented Aug 19, 2024

There is no need to call into WMI for most of these, there are available native APIs.

@sebhildebrandt
Copy link
Owner

@sylveon ... well yes. But this would require some external dependencies (like ffi, node-ffi, sbffi or others) plus some C code ... which I always wanted to avoid.

@dongle-the-gadget
Copy link

I don't think avoiding native code is the best choice here. WMI is already resource intensive, per se.

@WamWooWam
Copy link

hi, as the OP of that tweet i did want to apologise for anything that might've come your way as a result, did not remotely expect the reception it got and had i anticipated that i would've been much less harsh

that said, i do echo some of the sentiment here, the correct way to read this information on windows is programmatically via C-based apis, which is effectively what you're doing with powershell, just... slowly. as im sure you well know by now powershell can have a lot of launch and runtime overhead on slower or ultra-low-power processors (as was the case for me), especially when many instances are launched at once (as node's async model tends to do).

a set of native bindings would likely be a better long-term solution though i very much understand the difficulty that comes with that, maybe for a separate project?

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

5 participants