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

Saving boot time during ProcessesWithContext call (linux). #1497

Closed
wants to merge 2 commits into from

Conversation

bjandras
Copy link

@bjandras bjandras commented Aug 7, 2023

On Linux, the system boot time is used when calculating process creation times (expressed as number of seconds since the epoch). Obtaining the boot time involves parsing the /proc/stat file, which can incur siginificant overhead on systems with many CPU cores.

Permanently caching the boot time is not an option since it can change when the system clock is updated (e.g. ntpd).

In this changeset we will save the boot time during one invocation of the ProcessesWithContext function (listing all running processes).

@bjandras
Copy link
Author

bjandras commented Aug 7, 2023

This addresses issues #1070 and #1283.

To my knowledge, obtaining the boot time from /proc/stat is the only reliable
method. Alternative of subtracting the uptime from the current time is not
reliable since it requires obtaining these two times simultaneously.

@bjandras
Copy link
Author

bjandras commented Aug 7, 2023

Simple benchmark (just calling process.Processes() for testing.B.N times:

before:

goos: linux
goarch: amd64
pkg: testtest
cpu: Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
BenchmarkBTime-4             196           6110134 ns/op
PASS
ok      testtest        2.272s

with change:

goos: linux
goarch: amd64
pkg: testtest
cpu: Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz
BenchmarkBTime-4             445           2677522 ns/op
PASS
ok      testtest        1.480s

(depends heavily on the number of processes running)

@bjandras bjandras marked this pull request as ready for review August 7, 2023 18:35
On Linux, the system boot time is used when calculating process creation times
(expressed as number of seconds since the epoch). Obtaining the boot time
involves parsing the /proc/stat file, which can incur siginificant overhead
on systems with many CPU cores.

Permanently caching the boot time is not an option since it can change when the
system clock is updated (e.g. ntpd).

In this changeset we will save the boot time during one invocation of the
ProcessesWithContext function (listing all running processes).
Copy link
Owner

@shirou shirou left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. However, putting boot time information into the context is difficult to be accepted as an OSS library.

We request that putting the value in the context be handled by the user of gopsutil.

and also, #1283 is related.

@bjandras bjandras requested a review from shirou August 10, 2023 13:27
@bjandras
Copy link
Author

Requested changes have been implemented.

@shirou
Copy link
Owner

shirou commented Aug 16, 2023

Ah, I am sorry that I have misled you. What I meant was that caching in context should be done by the Application that uses gopsutil, not in the gopsutil, because it is a Library. To use a different term, we are unable to accept this PR.

@shirou shirou closed this Aug 16, 2023
@bjandras
Copy link
Author

@shirou I don't understand -- caching is done by the application, the library only uses the cached value when available.

@shirou
Copy link
Owner

shirou commented Aug 17, 2023

We do not want to add more context values. And also, bootTime can jump if time gets adjusted, described in this comment.

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

Successfully merging this pull request may close these issues.

3 participants