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

Process memory Enhancement [Linux] #1377

Open
sigsegved opened this issue Nov 8, 2024 · 10 comments
Open

Process memory Enhancement [Linux] #1377

sigsegved opened this issue Nov 8, 2024 · 10 comments

Comments

@sigsegved
Copy link

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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

@sigsegved
Copy link
Author

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
Copy link
Author

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

@GuillaumeGomez
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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

@sigsegved
Copy link
Author

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants