GuillaumeGomez / sysinfo

Cross-platform library to fetch system information
MIT License
2.1k stars 313 forks source link

Process memory Enhancement [Linux] #1377

Open sigsegved opened 1 week ago

sigsegved commented 1 week ago

I noticed sysinfo uses /proc/<pid>/statm to get the memory details like virtual memory and rss. As the man page suggests the rss reported in statm is not accurate and suggests using smaps_rollup instead. smaps_rollup file is pretty small one and provides a lot more information like PSS.

In reality, pss is a lot more useful memory metric than rss when there's a lot of shared memory involved in the system. So by changing where we get the memory stats from we can actually kill two birds in one stone. One caveat however is that, windows has no concept of PSS and I am not sure how we can expose these additional metrics available only in "unix" systems.

Maybe we can support process.memory_ext() that provides all the fields from smaps_rollup on demand.

Thoughts?

GuillaumeGomez commented 1 week ago

Seems like smaps_rollup needs more rights in order to be able to be read. Seems like until it's available all the time, we can't rely on this file unfortunately...

sigsegved commented 1 week ago

Yes smaps_rollup does require the process to run in privileged mode. Some of us use the sysinfo library in privileged mode anyway, wouldn't this benefit the folks who are okay with running in privileged mode?

GuillaumeGomez commented 1 week ago

It requires more code in order to work so I'm not very much in favour of it.

sigsegved commented 1 week ago

True that any new feature requires more code to work, but what is it about the code we will add for this feature that makes you go not in favor of it?

sigsegved commented 1 week ago

Btw i really appreciate the work you have done here. Big fan of this library.

GuillaumeGomez commented 1 week ago

True that any new feature requires more code to work, but what is it about the code we will add for this feature that makes you go not in favor of it?

We need to read the new file and if it fails, fall back to the current one. Which means that for your processes, you'll get different kind of information than others'. That also means that we'll try to read even more file compared to the current situation. Another approach would be to detect if the current user has access to all of these files, but I don't really see how since I'm not sure root users can have access to everything, including other root users processes.

sigsegved commented 1 week ago

I am proposing something slightly different than what you are envisioning. Assume we introduce memory_ext: HashMap<String, String> in the ProcessInner struct. ProcessInner will continue to have the current memory fields and they will be read from statm struct. we can introduce a new ProcessRefreshKind say memory_ext, only when this refreshkind is set we will go read all the smaps_rollup file and populate them in the HashMap.

With this approach, you can provide consistent behavior across platforms and allow each platform to provide their own "memory extension counters" if need arises.

GuillaumeGomez commented 1 week ago

But then it's something only available on linux targets, which is pretty bad for a unified API.

sigsegved commented 1 week ago

Is that so bad? Others will just get an empty HashMap and as long as it's well documented, do we see an issue?

I would argue that if some platforms provide better more accurate metrics, we should provide that information even if its optional only to that platform.

GuillaumeGomez commented 1 week ago

Well, that's two reasons not in favour of adding this feature. Adding more API if only one OS can use it is really something I want to avoid as much as possible.