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

Refresh thread stats info only if asked #1432

Closed
sigsegved opened this issue Dec 18, 2024 · 18 comments
Closed

Refresh thread stats info only if asked #1432

sigsegved opened this issue Dec 18, 2024 · 18 comments

Comments

@sigsegved
Copy link
Contributor

sigsegved commented Dec 18, 2024

Looks like the linux process refresh mechanism by default parses all stat files in /proc/<pid>/task/<tid>/stat. This can be expensive for highly threaded applications like nginx which could be using 100s of threads for async io. In most places a user might just be interested in the overall process information without requiring to know all the thread/task stat.

Would it be possible to have a new RefreshKind that supports refreshing only the pid stat?

@GuillaumeGomez
Copy link
Owner

Definitely sounds like a good idea.

@sigsegved
Copy link
Contributor Author

Since i know nothing about windows side of things how would you support the same for Windows?

@sigsegved
Copy link
Contributor Author

Side Note:
I also noticed that we skip processing /proc/<pid>/task/<pid>/stat to avoid double processing but in fact these two stat files will contain different data.

So /proc/<pid>/stat will contains stats like "cpu time" by the whole process but /proc/<pid>/task/<pid>/stat file will contain only the main thread's stat.

So for example: if the process is consuming 1000 clock ticks, and there are 10 threads each consuming 50 clock ticks, then you will see the the main thread will have the rest of the 500 clock ticks. by parsing just the main thread you can easily find the split of cpu usage between main thread and the rest.

@GuillaumeGomez
Copy link
Owner

So seems like it's still needed.

@sigsegved
Copy link
Contributor Author

@GuillaumeGomez Any thoughts on

Since i know nothing about windows side of things how would you support the same for Windows?

@GuillaumeGomez
Copy link
Owner

It's a linux-only feature, so basically this "refresh kind" would do nothing on other platforms.

@sigsegved
Copy link
Contributor Author

master...sigsegved:sysinfo:master i started working on this but want to check with you if this is going in the right direction.

@GuillaumeGomez
Copy link
Owner

Seems good to me, but just so you know: it means a lot of processes will not be listed anymore.

@sigsegved
Copy link
Contributor Author

why would that be the case? I thought all "processes" should still be there, but their "tasks" will be none. Right?

@sigsegved
Copy link
Contributor Author

@GuillaumeGomez waiting on your response above :)

@GuillaumeGomez
Copy link
Owner

I meant in case the thread refresh is disabled.

@sigsegved
Copy link
Contributor Author

@GuillaumeGomez updated the PR with the feedback. Please take a look.

#1436

@sigsegved
Copy link
Contributor Author

#1436

@GuillaumeGomez
Copy link
Owner

Fixed in #1436.

@sigsegved
Copy link
Contributor Author

@GuillaumeGomez Thanks for helping close this feature. What's the release cadence for sysinfo? I see 0.33.1 was released 12/27. Would we able to get 0.33.2 anytime soon? Happy to send out a PR for that as well.

@GuillaumeGomez
Copy link
Owner

This is a semver breaking change so it will require to be 0.34, meaning it will need some more time as I am working on other breaking changes.

@sigsegved
Copy link
Contributor Author

Gotcha. Thanks for the update. Any idea when we can expect the 0.34 version to be released?

@GuillaumeGomez
Copy link
Owner

No clue. I'm just back to work, so likely a few weeks minimum.

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