pkg/discovery: systemd discoverer with less allocs

[marselester]

Hi! I would like to share some of the results I got with https://github.com/marselester/systemd. There are still things I need to finish though...

The current version of discovery.SystemdDiscoverer makes use of 1.1 MB/op and 32,332 allocs/op to get a list of systemd services along with their PIDs. That accounts for 40% of CPU cycles in the agent, excluding the GC time, see https://pprof.me/e711789. The proposed change gets 13.96 KB/op and 30 allocs/op.

BenchmarkSystemd-2    	       1	1001314916 ns/op	 1163552 B/op	   32332 allocs/op
BenchmarkSystemd2-2   	       1	1003780290 ns/op	   14304 B/op	      30 allocs/op

I ran the benchmarks as follows:

• temporarily removed pkg/discovery/kubernetes.go or else tests won't run github.com/parca-dev/parca-agent/pkg/cgroup: build constraints exclude all Go files in /vagrant/parca-agent/pkg/cgroup
• placed the benchmarks into pkg/discovery/systemd_test.go
• sudo go test -run=^$ -bench=. -benchmem .

Details

// The benchmarks are not pretty, but at least they give a rough idea.

func BenchmarkSystemd(b *testing.B) {
	ctx := context.Background()
	conf := NewSystemdConfig()
	d, err := conf.NewDiscoverer(DiscovererOptions{})
	if err != nil {
		b.Fatal(err)
	}

	up := make(chan<- []*Group)
	ctx, cancel := context.WithTimeout(ctx, time.Second)
	b.Cleanup(cancel)

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		if err = d.Run(ctx, up); err != nil && err != context.DeadlineExceeded {
			b.Fatal(err)
		}
	}
}

func BenchmarkSystemd2(b *testing.B) {
	ctx := context.Background()
	conf := NewSystemd2Config()
	d, err := conf.NewDiscoverer(DiscovererOptions{})
	if err != nil {
		b.Fatal(err)
	}

	up := make(chan<- []*Group)
	ctx, cancel := context.WithTimeout(ctx, time.Second)
	b.Cleanup(cancel)

	b.ResetTimer()
	for i := 0; i < b.N; i++ {
		if err = d.Run(ctx, up); err != nil && err != context.DeadlineExceeded {
			b.Fatal(err)
		}
	}
}

I've added fmt.Printf to see whether the current implementation and a new one show the same services/PIDs.
I also tried to stop and start a cron service to make sure that was picked up in the updates.
The output matches.

It would be cool if you guys could have a look and give it a go in a safe environment.

$ make && sudo ./dist/parca-agent 2>&1 | grep --invert-match 'libbpf'
systemd-timesyncd.service 459
[email protected] 1468
systemd-networkd.service 607
unattended-upgrades.service 748
systemd-logind.service 675
dbus.service 663
systemd-journald.service 364
systemd-udevd.service 405
irqbalance.service 668
snapd.service 673
udisks2.service 676
cron.service 36684
polkit.service 32180
systemd-resolved.service 609
packagekit.service 15442
multipathd.service 402
networkd-dispatcher.service 669
[email protected] 686
[email protected] 683
ModemManager.service 32184
ssh.service 711
rsyslog.service 671

[marselester]

I've finished working on the systemd package and updated the PR, please review.

[kakkoyun]

LGTM ❤️ 💚 💙 💛 💜

I need more time to read the code in https://github.com/marselester/systemd

However, it seems like we have the parity and the benchmark numbers look great 🚀

[javierhonduco]

Looks amazing! Thanks so much for the very thorough work here.

There are still things I need to finish though...

I assume this comment is not up to date, right? :D

Happy to land this change and test it in our machines and in our Demo environment 😄

[brancz]

Since the scope is dramatically reduced, would it make sense to move the systemd package into the parca-agent repo? Happy to discuss that as a follow up though ...

[marselester]

I assume this comment is not up to date, right? :D

@javierhonduco that's right :)

[marselester]

@brancz sure, that can be done. It would be good to keep README.md and bench-old.txt within a package to retain a context. Another option is to fork the repository.

[javierhonduco]

Let's merge this to include it in the release we are cutting today and we can fork/move the code later on