gopsutil icon indicating copy to clipboard operation
gopsutil copied to clipboard

Add NewProcessWithFields and ProcessesWithFields

Open PierreF opened this issue 4 years ago • 9 comments

PR for #286, which add NewProcessWithFields and ProcessesWithFields. All information are fetched upfront (so no update) and using fewer syscall/files read than the NewProcess/Processes conterpart.

Information must be requested when calling constructor or ErrorFieldNotRequested is returned (e.g. you can't call Ppid() if you didn't passed FieldPpid to NewProcessWithFields)

Parent() and Children() do not use pre-fetched information, but Process object returned are created using NewProcessWithFields with the same fields. We don't pre-fetch those fields because it would recursively fetch all the parents, grand-parents, ... (or children, grand-children, ...). If both are activated it would recursively fetch all processes.

The way it's done is:

  • Keep the list of fields requested. On each public method (like Ppid(), CreateTime()...) check that the fields was requested or return ErrorFieldNotRequested
  • Add a cache which if nil means use classic NewProcess way and fetch on demand
  • If cache is non-nil, then use the cached value if present. If absent, fetch the information and update the cache.
  • The cache is added on low-level function, like the function that read /proc/PID/stat. Since this function is used by multiple public method (Ppid(), Times(), CreateTime()...) the cache will ensure this file is only read once for all public method call.
  • The cache is pre-filled when calling the constructor by calling public method for requested fields, which will fill the cache.

To run tests, I've done a fix for #879. I'm running Ubuntu 20.04 and newer kernel added THPeligible in smaps which use tab instead of space :(

I've run tests on (host is amd64, run amd64 and 386 build)

  • Linux 5.4.0 (Ubuntu 20.04)
  • Windows 10
  • FreeBSD 12.1
  • OpenBSD (only amd64, because 386 seems broken)

On those machine, I've also run

  • ps1.go: a "ps-like" which use gopsutil and works with master & this PR
  • ps2.go: a "ps-like" which use gopsutil and can make usage of ProcessesWithFields

For ps1, I've run it with "go run ps1.go -count 1 -show" from both master and the PR to check that output is the same.

I've also run ps1 with default option (no show & count = 10) to confirm this PR does not reduce performance of existing users:

linux $ ./ps1-master
errCount = 0. Spent 11.457741013s
linux $ ./ps1-pr
errCount = 0. Spent 11.357042901s

For ps2, I've run it with and without -fields to (using also -show) confirm that output is the same and to see performance gain:

linux $ ./ps2
errCount = 0. Spent 10.022242972s
linux $ ./ps2 -fields
errCount = 0. Spent 3.214356181s

PierreF avatar Jun 15 '20 14:06 PierreF

Continued works. Linux should be completed.

Children field have the same issue as Parent field: if we prefetch this field and create children's Process with same pre-fetched fields, it may retrieve all processes. I'm not sure what we want here, so currently both Parent and Children is not pre-fetched, and when called, the PIDs will be fetched and parent/children Process are created using same pre-fetched fields.

I've use a basic "ps" that use gopsutils to validate the this PR doesn't impact performance of NewProcess. Performance is unchanged:

$ \time ./ps.master
10.01user 13.03system 0:18.44elapsed 124%CPU (0avgtext+0avgdata 13636maxresident)k
0inputs+88outputs (0major+7431minor)pagefaults 0swaps

$ \time ./ps.new
9.63user 12.79system 0:17.79elapsed 126%CPU (0avgtext+0avgdata 13584maxresident)k
0inputs+96outputs (0major+7419minor)pagefaults 0swaps

Note: this is when not using NewProcessWithFields, i.e. existing user when pay any performance penalty for this PR.

PierreF avatar Jun 16 '20 21:06 PierreF

Add support for Windows and run test on Windows 10.

Performance are still not impacted for NewProcess (still using basic "ps"):

C:\Users\IEUser\Downloads>ps-master.exe -count 100
errCount = 4300. Spent 435.0854ms

C:\Users\IEUser\Downloads>ps-new.exe -count 100
errCount = 4300. Spent 433.0671ms

(the errCount is due to trying to call not implemented function, like Terminal or Status)

PierreF avatar Jun 17 '20 21:06 PierreF

Added ProcessesWithFields on Linux & Windows.

I've make ProcessesWithFields to take a context as argument. I think both ProcessesWithFields and NewProcessWithFields should both take a context or none. But I'm unsure which way we are moving. Are we adding context to all function (and ThingWithContext is the way to go) or are we removing context from all function.

I've updated my "basic" ps to support WithFields. It much better on Linux. Windows is not that better:

$ ./ps 
errCount = 0. Spent 9.417733979s
$ ./ps -fields
errCount = 0. Spent 2.993156593s

C:\Users\pierref\Downloads>ps.exe -count 100
errCount = 44100. Spent 2.9082738s

C:\Users\pierref\Downloads>ps.exe -count 100 -fields
errCount = 44100. Spent 2.8075755s

PierreF avatar Jun 19 '20 15:06 PierreF

This PR should be completed now. Waiting for review.

PierreF avatar Jun 22 '20 17:06 PierreF

Thank you for this PR, it seems like it would be very useful to my for my project in which procs take most of the compute time. Could we cache internal/common.Virtualization() function after first ever call to it as well? It seems that there isn't much reason to call it again as the OS and Virtualization information won't change.

I will reduce number of NewProcess calls in my code but here is the profiling tree image

AtakanColak avatar Sep 10 '20 11:09 AtakanColak

@AtakanColak caching common.Virtualization() may be for another PR/issue.

@PierreF sorry dropped the ball on this, life got in the way and then forgot about this specific PR. That's quite a big change but also an improvement as highlighted also by @AtakanColak. I don't know when I will be able to review it (I had a smaller change in mind, but yours is more optimized probably).

Lomanic avatar Sep 10 '20 19:09 Lomanic

@PierreF I also would like to make a suggestion regarding this PR. While calling NewProcessWithFields() we are spending some time on fetching requested information from the OS. However in that meantime, the process can exit, and even its PID can get replaced by a newer process. For this, I believe there should be a simple CreateTime() check at the end of the NewProcessWithFields() function.

start := time.now() // before fetching // fetch requested fields crt, err := p.CreateTime() // after fetching if err != nil // if we can't get create time at this point maybe we should return this error if crt > (start.UnixNano() / 1000000) // because CreateTime() return in milliseconds and UnixNano is nanoseconds // if crt is after we started, then return an error saying process was changed in the meantime

AtakanColak avatar Sep 11 '20 08:09 AtakanColak

when will be this pull request getting merged?

Purushothama-VS avatar Apr 04 '22 08:04 Purushothama-VS

Continued works. Linux should be completed.

Children field have the same issue as Parent field: if we prefetch this field and create children's Process with same pre-fetched fields, it may retrieve all processes. I'm not sure what we want here, so currently both Parent and Children is not pre-fetched, and when called, the PIDs will be fetched and parent/children Process are created using same pre-fetched fields.

I've use a basic "ps" that use gopsutils to validate the this PR doesn't impact performance of NewProcess. Performance is unchanged:

$ \time ./ps.master
10.01user 13.03system 0:18.44elapsed 124%CPU (0avgtext+0avgdata 13636maxresident)k
0inputs+88outputs (0major+7431minor)pagefaults 0swaps

$ \time ./ps.new
9.63user 12.79system 0:17.79elapsed 126%CPU (0avgtext+0avgdata 13584maxresident)k
0inputs+96outputs (0major+7419minor)pagefaults 0swaps

Note: this is when not using NewProcessWithFields, i.e. existing user when pay any performance penalty for this PR.

when do you plan to merge this pull request?

Purushothama-VS avatar Apr 04 '22 08:04 Purushothama-VS