Cache common/common_linux.Virtualization()

[AtakanColak]

By assuming virtualization environment won't change during a the program's runtime, we can cache common/common_linux.Virtualization() with a simple map to reduce amount of system calls. I first mentioned this issue at 890

[AtakanColak]

I wanted to compare profiling results of this optimization.

Before optimization of Virtualization(), there is a distinct compute weight on fillFromStatWithContext which calls Virtualization()

After optimization, we can clearly see that the compute weight is reduced and spread to the other functions.

[shirou]

I think these kind of optimization should be implemented in Application side, not library side.

[AtakanColak]

@shirou could you elaborate on that? Most of the compute is spent with ReadLines on Virtualization which is in internal/common package. How could we optimize it from application side?

[shirou]

package main

import (
        "fmt"
        "github.com/shirou/gopsutil/host"
)

var (
        h = ""
        g = ""
)

func main() {
        if h == "" {
                h, g, _ := host.Virtualization()
                fmt.Println(h, g)
        } else {
                fmt.Println(h, g)
        }
}

or whatever, in your application.

[AtakanColak]

@shirou thank you for your reply. However your answer would be the case if it was my app who was calling Virtualization information, but it isn't. The function that calls common/common_linux.Virtualization() is fillFromStatWithContext() which is called by many process.Process methods in process_linux. We can't change private functions from the application side.

[Lomanic]

In fact the call to common.Virtualization() (should be VirtualizationWithContext() probably also) comes from common.BootTimeWithContext() which is not cached anymore since #857. As I wrote in #857 (comment), moving to unix.Sysinfo would maybe remove this call.

I fear caching this will make it harder to run unit tests for this function (we would have to hook to this new variable virtualizationCache I think).

[shirou]

ah, sorry for my misunderstanding. But as @Lomanic said, I also cache is not good for library. I will try to open a PR about using unix.Sysinfo

[AtakanColak]

@Lomanic #857 mention a device can start with wrong date but the question we must ask for this case is can a device start with wrong virtualization data? I don't think it can but of course we must be certain.

I'm not going to comment on unit tests as I'm not sure on potential effects of caching on unit tests.

[Lomanic]

I think maybe we should cache this value in fact, for unit tests we can work around this by resetting this internal variable to nil if we want to call Virtualization() more than once. This would really improve performances for process.Processes() (see #842 (comment))

[Lomanic]

Here's a proper benchmark

git -C "$(go env GOPATH)/src/github.com/shirou/gopsutil" checkout master
git -C "$(go env GOPATH)/src/github.com/shirou/gopsutil" reset --hard origin/master
git -C "$(go env GOPATH)/src/github.com/shirou/gopsutil" clean -f -d
cat <<EOF >> "$(go env GOPATH)/src/github.com/shirou/gopsutil/process/process_test.go"
func BenchmarkNewProcess(b *testing.B) {
	checkPid := os.Getpid()
	for i := 0; i < b.N; i++ {
		NewProcess(int32(checkPid))
    }
}
EOF
printf "\nBefore change:\n"
go test -bench=. github.com/shirou/gopsutil/process
curl -sL https://github.com/shirou/gopsutil/pull/942.diff | git apply
printf "\nAfter change:\n"
go test -bench=. github.com/shirou/gopsutil/process

Results are telling:

Before change:
goos: linux
goarch: amd64
pkg: github.com/shirou/gopsutil/process
BenchmarkNewProcess-4               2000            542412 ns/op
PASS
ok      github.com/shirou/gopsutil/process      3.106s

After change:
goos: linux
goarch: amd64
pkg: github.com/shirou/gopsutil/process
BenchmarkNewProcess-4              20000             78478 ns/op
PASS
ok      github.com/shirou/gopsutil/process      4.158s

[AtakanColak]

removed explicit nil

[AtakanColak]

@Lomanic removed explicit nil as requested. Thank you for benchmarking results. Looking forward for the merge.

[shirou]

As I said, I don't like cache in a library, but I don't have any other good solution, so I approve this PR. Thank you for your contribution!

[reimda]

I tested this fix in telegraf and found that it makes performance similar to before #837 was merged. Will this PR make it into a release soon? We'd like to include it in telegraf 1.16.0 which is planned for next week.

[shirou]

@Lomanic sorry for interrupting your comment, but I think your request is resolved. So I merge this PR. Thank you for your review and contribution!

[shirou]

gopsutil is monthly release. After this PR merged, I have released v2.20.9.