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

Fix KB conversion for memory in Windows #696

Conversation

oraclerob
Copy link

Fix to divide the bytes output from the GlobalMemoryStatusEx(&mut mem_info) function by 1024 to get the correct KB usage for the system.

self.swap_total = (swap_total / 1000) as u64;
self.swap_used = (swap_used / 1000) as u64;
self.swap_total = (swap_total / 1024) as u64;
self.swap_used = (swap_used / 1024) as u64;
Copy link
Owner

Choose a reason for hiding this comment

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

Based on https://docs.microsoft.com/en-us/windows/win32/api/psapi/ns-psapi-performance_information, PageSize is in bytes so this conversion is invalid.

self.mem_total = auto_cast!(mem_info.ullTotalPhys, u64) / 1_000;
self.mem_available = auto_cast!(mem_info.ullAvailPhys, u64) / 1_000;
self.mem_total = auto_cast!(mem_info.ullTotalPhys, u64) / 1_024;
self.mem_available = auto_cast!(mem_info.ullAvailPhys, u64) / 1_024;
Copy link
Owner

Choose a reason for hiding this comment

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

And based on this page: https://docs.microsoft.com/en-us/windows/win32/api/sysinfoapi/ns-sysinfoapi-memorystatusex, it's all in bytes so this conversion is unneeded as well.

@GuillaumeGomez
Copy link
Owner

An important thing to note: sysinfo is using KB, not KiB, hence the 1000 divisions.

@oraclerob
Copy link
Author

Yes, but your own example is listing these in KB and you end up with more memory than is actually installed. It doesn't look right. This is a contentious issue, but most systems assume a KB is 1024 bytes and not 1000 but I know there is a metric equivalent. Up to you. but on my Windows system I end up with more memory listed than installed. I'll pull the PR.

println!("total memory: {} KB", sys.total_memory());
println!("used memory : {} KB", sys.used_memory());
println!("total swap : {} KB", sys.total_swap());
println!("used swap : {} KB", sys.used_swap());

@GuillaumeGomez
Copy link
Owner

A KB is never 1024 unless it is a kikibyte (as opposed to kilobyte). There is a big confusion around KiB and KB because sometimes KiB is written Kb (even saw KB...).

Yes, but your own example is listing these in KB and you end up with more memory than is actually installed.

Do you have a screenshot with the complete display of your installed RAM? Because they tend to round it.

@oraclerob
Copy link
Author

image

If you have a look at this wiki page, it does say at the bottom that MS tend to display in 1024 units.

https://en.wikipedia.org/wiki/Kilobyte

To be honest, I can't be bothered arguing about this and I'm happy to close this as I can spend my time on other projects. Incorporating windows-rs crate into this project (which API call gives me the correct answer) means we will need to rename the windows directory because there is a name conflict in the lib.rs namespace so I thought this would be the easiest option to display the correct amount of windows memory. Sadly I won't be able to use this tool for windows.

@GuillaumeGomez
Copy link
Owner

To be honest, I can't be bothered arguing about this and I'm happy to close this as I can spend my time on other projects. Incorporating windows-rs crate into this project (which API call gives me the correct answer) means we will need to rename the windows directory because there is a name conflict in the lib.rs namespace so I thought this would be the easiest option to display the correct amount of windows memory. Sadly I won't be able to use this tool for windows.

Except the memory returned by sysinfo is valid, just not in the unit you want. The conversion between the two is pretty simple though: mem * 1000 / 1024. It seems pretty minor but in the end it's up to you. sysinfo explicitly provides memory size in kilobytes so there is no reason that Windows would stand on its own apart from the other systems in that regard.

I'll be closing this then. Thanks for taking the time to look into it.

@NaokiM03
Copy link

NaokiM03 commented Mar 6, 2022

@GuillaumeGomez

Hello.
Sorry for my poor English.

I also faced the same problem.
The Examples below divide by 1024 to convert bytes to KB. So, I think it would be better to use 1024 instead of 1000 in Windows.

// Use to convert bytes to KB
#define DIV 1024

https://docs.microsoft.com/en-us/windows/win32/api/sysinfoapi/nf-sysinfoapi-globalmemorystatusex

@GuillaumeGomez
Copy link
Owner

It's not KB, it's KiB. A common confusion, so no. If you want KiB, then * 1000 / 1024.

@NaokiM03
Copy link

NaokiM03 commented Mar 6, 2022

Okay.
On Windows, KiB is almost always written as KB.
I understood that the decision was made not to follow that convention for this crate.

I'm sorry, if it's KB, it should be divided by 1000. I was wrong.

@GuillaumeGomez
Copy link
Owner

Windows documentation is making this very awkward and I find it really bad for readers comprehension but not much we can do about it unfortunately...

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.

None yet

3 participants