libcontainer/cgroups/fs: remove todo since strings.Fields performs well #4403

Stavrospanakakis · 2024-09-15T14:06:18Z

Initially, this was a commit to switch from strings.Fields to
strings.SplitN in getCpuUsageBreakdown, since strings.Fields
was probably slower than strings.SplitN in some old Go versions.

Afterwards, strings.Cut was also considered for potential
speed improvements.

After writing a benchmark test, we learned that:

strings.Fields performance is now adequate;
strings.SplitN is slower than strings.Fields;
strings.Cut had <5% performance gain from strings.Fields;

So, remove the TODO and keep the benchmark test.

The following are benchmark differences using strings.Fields,
strings.SplitN, and strings.Cut.

Below, you can find the benchmark differences.

# original implementation
$ go test -run 1234 -bench . -benchmem -count 8 . > strings.Fields

# update the implementation to use strings.SplitN
$ vim cpuacct.go 
$ go test -run 1234 -bench . -benchmem -count 8 . > strings.SplitN

# update the implementation to use strings.Cut
$ vim cpuacct.go 
$ go test -run 1234 -bench . -benchmem -count 8 . > strings.Cut

# compare the implementations
$ benchstat strings.Fields strings.SplitN strings.Cut
goos: linux
goarch: amd64
pkg: github.com/opencontainers/runc/libcontainer/cgroups/fs
cpu: QEMU TCG CPU version 2.5+
                       │ strings.Fields │          strings.SplitN           │            strings.Cut            │
                       │     sec/op     │   sec/op     vs base              │   sec/op     vs base              │
GetCpuUsageBreakdown-5      15.78µ ± 1%   16.51µ ± 1%  +4.62% (p=0.000 n=8)   15.56µ ± 2%  -1.39% (p=0.050 n=8)

                       │ strings.Fields │           strings.SplitN           │            strings.Cut             │
                       │      B/op      │     B/op      vs base              │     B/op      vs base              │
GetCpuUsageBreakdown-5     1.945Ki ± 0%   1.977Ki ± 0%  +1.61% (p=0.000 n=8)   1.883Ki ± 0%  -3.21% (p=0.000 n=8)

                       │ strings.Fields │           strings.SplitN           │            strings.Cut            │
                       │   allocs/op    │  allocs/op   vs base               │ allocs/op   vs base               │
GetCpuUsageBreakdown-5      10.000 ± 0%   12.000 ± 0%  +20.00% (p=0.000 n=8)   9.000 ± 0%  -10.00% (p=0.000 n=8)

Different implementations

Below, you can find the different implementations.

`strings.Fields`

// Returns user and kernel usage breakdown in nanoseconds.
func getCpuUsageBreakdown(path string) (uint64, uint64, error) {
	var userModeUsage, kernelModeUsage uint64
	const (
		userField   = "user"
		systemField = "system"
		file        = cgroupCpuacctStat
	)

	// Expected format:
	// user <usage in ticks>
	// system <usage in ticks>
	data, err := cgroups.ReadFile(path, file)
	if err != nil {
		return 0, 0, err
	}
	// TODO: use strings.SplitN instead.
	fields := strings.Fields(data)
	if len(fields) < 4 || fields[0] != userField || fields[2] != systemField {
		return 0, 0, malformedLine(path, file, data)
	}
	if userModeUsage, err = strconv.ParseUint(fields[1], 10, 64); err != nil {
		return 0, 0, &parseError{Path: path, File: file, Err: err}
	}
	if kernelModeUsage, err = strconv.ParseUint(fields[3], 10, 64); err != nil {
		return 0, 0, &parseError{Path: path, File: file, Err: err}
	}

	return (userModeUsage * nanosecondsInSecond) / clockTicks, (kernelModeUsage * nanosecondsInSecond) / clockTicks, nil
}

`strings.SplitN`

// Returns user and kernel usage breakdown in nanoseconds.
func getCpuUsageBreakdown(path string) (uint64, uint64, error) {
	var userModeUsage, kernelModeUsage uint64
	const (
		userField   = "user"
		systemField = "system"
		file        = cgroupCpuacctStat
	)

	// Expected format:
	// user <usage in ticks>
	// system <usage in ticks>
	data, err := cgroups.ReadFile(path, file)
	if err != nil {
		return 0, 0, err
	}

	lines := strings.SplitN(data, "\n", 2)
	if len(lines) < 2 {
		return 0, 0, malformedLine(path, file, data)
	}

	userLine := lines[0]
	userLineFields := strings.SplitN(userLine, " ", 2)
	if len(userLineFields) != 2 || userLineFields[0] != userField {
		return 0, 0, malformedLine(path, file, data)
	}

	userUsageInTicks := strings.TrimSpace(userLineFields[1])
	if userModeUsage, err = strconv.ParseUint(userUsageInTicks, 10, 64); err != nil {
		return 0, 0, &parseError{Path: path, File: file, Err: err}
	}

	systemLine := lines[1]
	systemLineFields := strings.SplitN(systemLine, " ", 2)
	if len(systemLineFields) != 2 || systemLineFields[0] != systemField {
		return 0, 0, malformedLine(path, file, data)
	}

	systemUsageInTicks := strings.TrimSpace(systemLineFields[1])
	if kernelModeUsage, err = strconv.ParseUint(systemUsageInTicks, 10, 64); err != nil {
		return 0, 0, &parseError{Path: path, File: file, Err: err}
	}

	return (userModeUsage * nanosecondsInSecond) / clockTicks, (kernelModeUsage * nanosecondsInSecond) / clockTicks, nil
}

`strings.Cut`

// Returns user and kernel usage breakdown in nanoseconds.
func getCpuUsageBreakdown(path string) (uint64, uint64, error) {
	var userModeUsage, kernelModeUsage uint64
	const (
		userField   = "user"
		systemField = "system"
		file        = cgroupCpuacctStat
	)

	// Expected format:
	// user <usage in ticks>
	// system <usage in ticks>
	data, err := cgroups.ReadFile(path, file)
	if err != nil {
		return 0, 0, err
	}

	userLine, systemLine, found := strings.Cut(data, "\n")
	if !found {
		return 0, 0, malformedLine(path, file, data)
	}

	userFieldWord, userUsageInTicks, found := strings.Cut(userLine, " ")
	if !found || userFieldWord != userField {
		return 0, 0, malformedLine(path, file, data)
	}

	userUsageInTicks = strings.TrimSpace(userUsageInTicks)
	if userModeUsage, err = strconv.ParseUint(userUsageInTicks, 10, 64); err != nil {
		return 0, 0, &parseError{Path: path, File: file, Err: err}
	}

	systemFieldWord, systemUsageInTicks, found := strings.Cut(systemLine, " ")
	if !found || systemFieldWord != systemField {
		return 0, 0, malformedLine(path, file, data)
	}

	systemUsageInTicks = strings.TrimSpace(systemUsageInTicks)
	if kernelModeUsage, err = strconv.ParseUint(systemUsageInTicks, 10, 64); err != nil {
		return 0, 0, &parseError{Path: path, File: file, Err: err}
	}

	return (userModeUsage * nanosecondsInSecond) / clockTicks, (kernelModeUsage * nanosecondsInSecond) / clockTicks, nil
}

cyphar · 2024-09-15T22:35:39Z

Thanks for the patch! Would you mind writing a small benchmark to see if the extra splits aren't slower? (Though I guess this code isn't called more than a couple of times, so maybe it's not that big of a deal...)

Stavrospanakakis · 2024-09-16T08:18:38Z

@cyphar I created the 2 functions as func getCpuUsageBreakdownWithSplitN() error and func getCpuUsageBreakdownWithFields() error and mocked all the unnecessary parts.

Using strings.SplitN is a bit slower than using strings.Fields, but I believe using strings.SplitN makes the code more stable and clean.

What do you think?

Output:

$ go test -bench=.
goos: darwin
goarch: arm64
pkg: github.com/stavrospanakakis/benchtest
BenchmarkGetCpuUsageBreakdownWithFields-10      15189668                79.04 ns/op
BenchmarkGetCpuUsageBreakdownWithSplitN-10      11211726               105.7 ns/op
PASS
ok      github.com/stavrospanakakis/benchtest   4.155s

// bench.go
package bench

import (
	"errors"
	"strconv"
	"strings"
)

// Returns user and kernel usage breakdown in nanoseconds.
func getCpuUsageBreakdownWithSplitN() error {
	const (
		userField   = "user"
		systemField = "system"
	)

	// Expected format:
	// user <usage in ticks>
	// system <usage in ticks>
	data := "user 452278264\nsystem 291429664"

	lines := strings.SplitN(data, "\n", 2)
	if len(lines) < 2 {
		return errors.New("unexpected content in cgroup cpuacct.stat")
	}

	userLine := lines[0]
	userLineFields := strings.SplitN(userLine, " ", 2)
	if len(userLineFields) != 2 || userLineFields[0] != userField {
		return errors.New("unexpected content in cgroup cpuacct.stat")
	}

	userUsageInTicks := strings.TrimSpace(userLineFields[1])
	if _, err := strconv.ParseUint(userUsageInTicks, 10, 64); err != nil {
		return err
	}

	systemLine := lines[1]
	systemLineFields := strings.SplitN(systemLine, " ", 2)
	if len(systemLineFields) != 2 || systemLineFields[0] != systemField {
		return errors.New("unexpected content in cgroup cpuacct.stat")
	}

	systemUsageInTicks := strings.TrimSpace(systemLineFields[1])
	if _, err := strconv.ParseUint(systemUsageInTicks, 10, 64); err != nil {
		return errors.New("unexpected content in cgroup cpuacct.stat")
	}
	return nil
}

// Returns user and kernel usage breakdown in nanoseconds.
func getCpuUsageBreakdownWithFields() error {
	const (
		userField   = "user"
		systemField = "system"
	)

	// Expected format:
	// user <usage in ticks>
	// system <usage in ticks>
	data := "user 452278264\nsystem 291429664"

	// TODO: use strings.SplitN instead.
	fields := strings.Fields(data)
	if len(fields) < 4 || fields[0] != userField || fields[2] != systemField {
		return errors.New("unexpected content in cgroup cpuacct.stat")
	}

	if _, err := strconv.ParseUint(fields[1], 10, 64); err != nil {
		return err
	}

	if _, err := strconv.ParseUint(fields[3], 10, 64); err != nil {
		return err
	}

	return nil
}

// bench_test.go
package bench

import "testing"

func BenchmarkGetCpuUsageBreakdownWithFields(b *testing.B) {
	for i := 0; i < b.N; i++ {
		getCpuUsageBreakdownWithFields()
	}
}

func BenchmarkGetCpuUsageBreakdownWithSplitN(b *testing.B) {
	for i := 0; i < b.N; i++ {
		getCpuUsageBreakdownWithSplitN()
	}
}

kolyshkin · 2024-09-16T19:30:20Z

libcontainer/cgroups/fs/cpuacct.go

-	fields := strings.Fields(data)
-	if len(fields) < 4 || fields[0] != userField || fields[2] != systemField {
+
+	lines := strings.SplitN(data, "\n", 2)


In this case you can actually use strings.Cut (which was not available when this TODO was written).

I added a commit that uses strings.Cut instead of strings.SplitN and strings.Fields.

kolyshkin · 2024-09-16T19:30:36Z

libcontainer/cgroups/fs/cpuacct.go

+	}
+
+	userLine := lines[0]
+	userLineFields := strings.SplitN(userLine, " ", 2)


same here, strings.Cut will work better.

Same as above.

kolyshkin · 2024-09-16T19:32:08Z

@Stavrospanakakis do you mind rewriting the benchmark using https://pkg.go.dev/testing#hdr-Benchmarks and making it part of the PR?

kolyshkin · 2024-09-16T19:35:09Z

Also, I barely remember that strings.Fields was slower than strings.SplitN back in the day, but now this is fixed so maybe we just need to remove the TODO (after checking that the above is true), or check if using strings.Cut makes the code easier and/or faster, and switch to it.

Stavrospanakakis · 2024-09-16T22:55:13Z

@Stavrospanakakis do you mind rewriting the benchmark using https://pkg.go.dev/testing#hdr-Benchmarks and making it part of the PR?

@kolyshkin I added the benchmark test as a part of this pull request. Also, after running the test, strings.Cut is faster than strings.Fields.

go test -bench=.
goos: darwin
goarch: arm64
pkg: github.com/stavrospanakakis/benchtest
BenchmarkGetCpuUsageBreakdownWithFields-10    	14914494	        78.71 ns/op
BenchmarkGetCpuUsageBreakdownWithCut-10       	24691188	        48.50 ns/op
PASS
ok  	github.com/stavrospanakakis/benchtest	3.767s

kolyshkin · 2024-09-17T01:02:14Z

libcontainer/cgroups/fs/cpuacct.go

 		return 0, 0, malformedLine(path, file, data)
 	}
-	if userModeUsage, err = strconv.ParseUint(fields[1], 10, 64); err != nil {
+
+	userUsageInTicks = strings.TrimSpace(userUsageInTicks)


Are there extra spaces?

When I implemented the change with strings.SplitN, I received the following error from the tests, which is why I added the userUsageInTicks = strings.TrimSpace(userUsageInTicks) that fixed this error.

# time="2024-09-15T14:08:22Z" level=error msg="unable to get container cgroup stats: unable to parse /sys/fs/cgroup/cpu,cpuacct/system.slice/runner-provisioner.service/test_busybox/cpuacct.stat: strconv.ParseUint: parsing \"1\\n\": invalid syntax"

Ah fwiw that's probably because lines := strings.SplitN(data, "\n", 2) would only "eat" the middle newline where the file contents have two newlines (line1\nline2\n) and strings.Fields splits on any whitespace, including consuming consecutive and anything extra at the beginning and end, hence exactly 4 fields.

Yes, strings.Fields removes all whitespace around which is ideal for this use case.

kolyshkin · 2024-09-17T01:24:17Z

I used the benchmark (slightly modified, see below) and the code from this PR, and the gain looks much less dramatic (

[kir@kir-tp1 fs]$ perflock go test -run 1234 -bench . -benchmem -count 8 . > before
[kir@kir-tp1 fs]$ vim cpuacct.go 
[kir@kir-tp1 fs]$ perflock go test -run 1234 -bench . -benchmem -count 8 . > after
[kir@kir-tp1 fs]$ benchstat before after 
goos: linux
goarch: amd64
pkg: github.com/opencontainers/runc/libcontainer/cgroups/fs
cpu: 12th Gen Intel(R) Core(TM) i7-12800H
                        │   before    │               after               │
                        │   sec/op    │   sec/op     vs base              │
GetCpuUsageBreakdown-20   3.751µ ± 3%   3.589µ ± 2%  -4.32% (p=0.002 n=8)

                        │    before    │               after                │
                        │     B/op     │     B/op      vs base              │
GetCpuUsageBreakdown-20   1.945Ki ± 0%   1.883Ki ± 0%  -3.21% (p=0.000 n=8)

                        │   before    │               after               │
                        │  allocs/op  │ allocs/op   vs base               │
GetCpuUsageBreakdown-20   10.000 ± 0%   9.000 ± 0%  -10.00% (p=0.000 n=8)

So, given the performance difference and code complexity, we should probably keep strings.Fields as is, remove TODO, add a benchmark and describe in the commit message that strings.Fields gives us a decent performance.

Stavrospanakakis · 2024-09-17T10:10:53Z

I used the benchmark (slightly modified, see below) and the code from this PR, and the gain looks much less dramatic (

[kir@kir-tp1 fs]$ perflock go test -run 1234 -bench . -benchmem -count 8 . > before
[kir@kir-tp1 fs]$ vim cpuacct.go 
[kir@kir-tp1 fs]$ perflock go test -run 1234 -bench . -benchmem -count 8 . > after
[kir@kir-tp1 fs]$ benchstat before after 
goos: linux
goarch: amd64
pkg: github.com/opencontainers/runc/libcontainer/cgroups/fs
cpu: 12th Gen Intel(R) Core(TM) i7-12800H
                        │   before    │               after               │
                        │   sec/op    │   sec/op     vs base              │
GetCpuUsageBreakdown-20   3.751µ ± 3%   3.589µ ± 2%  -4.32% (p=0.002 n=8)

                        │    before    │               after                │
                        │     B/op     │     B/op      vs base              │
GetCpuUsageBreakdown-20   1.945Ki ± 0%   1.883Ki ± 0%  -3.21% (p=0.000 n=8)

                        │   before    │               after               │
                        │  allocs/op  │ allocs/op   vs base               │
GetCpuUsageBreakdown-20   10.000 ± 0%   9.000 ± 0%  -10.00% (p=0.000 n=8)

So, given the performance difference and code complexity, we should probably keep strings.Fields as is, remove TODO, add a benchmark and describe in the commit message that strings.Fields gives us a decent performance.

@kolyshkin I applied these changes to a new commit.

kolyshkin · 2024-09-18T02:16:16Z

It would be good to describe in a commit message what we learned here. Something like

Initially, this was a commit to switch from strings.Fields to
strings.SplitN in getCpuUsageBreakdown, but we learned that:

 - strings.Fields was probably slower than strings.Split[N] in
   some old Go versions;
 - it's performance is now adequate (using the new benchmark on versions
   using strings.SplitN and strings.Cut shows there is <5% performance gain);

So, remove the TODO (and keep the benchmark).

This is just an example to give you an idea of what would make sense to have in a commit message. If you like, you can give it some more time and actually show the benchmark differences between two (or three) versions.

Initially, this was a commit to switch from strings.Fields to strings.SplitN in getCpuUsageBreakdown, since strings.Fields was probably slower than strings.SplitN in some old Go versions. Afterwards, strings.Cut was also considered for potential speed improvements. After writing a benchmark test, we learned that: - strings.Fields performance is now adequate; - strings.SplitN is slower than strings.Fields; - strings.Cut had <5% performance gain from strings.Fields; So, remove the TODO and keep the benchmark test. Signed-off-by: Stavros Panakakis <[email protected]>

Stavrospanakakis · 2024-09-18T15:44:45Z

It would be good to describe in a commit message what we learned here. Something like
Initially, this was a commit to switch from strings.Fields to
strings.SplitN in getCpuUsageBreakdown, but we learned that:

 - strings.Fields was probably slower than strings.Split[N] in
   some old Go versions;
 - it's performance is now adequate (using the new benchmark on versions
   using strings.SplitN and strings.Cut shows there is <5% performance gain);

So, remove the TODO (and keep the benchmark).
This is just an example to give you an idea of what would make sense to have in a commit message. If you like, you can give it some more time and actually show the benchmark differences between two (or three) versions.

@kolyshkin Sure, I updated the commit message. I also updated the pull request description to include the benchmark differences and the different implementations.

lifubang

LGTM, thanks your work!

Stavrospanakakis force-pushed the replace-fields-splitn-cgroups-fs branch from bd59f35 to a1e7cca Compare September 15, 2024 14:44

kolyshkin reviewed Sep 16, 2024

View reviewed changes

Stavrospanakakis force-pushed the replace-fields-splitn-cgroups-fs branch from a1e7cca to 3518a94 Compare September 16, 2024 22:39

Stavrospanakakis changed the title ~~libcontainer/cgroups/fs: replace strings.Fields with strings.SplitN~~ libcontainer/cgroups/fs: replace strings.Fields with strings.Cut Sep 16, 2024

Stavrospanakakis requested a review from kolyshkin September 16, 2024 23:03

kolyshkin reviewed Sep 17, 2024

View reviewed changes

Stavrospanakakis force-pushed the replace-fields-splitn-cgroups-fs branch 3 times, most recently from 3816b28 to 9a37150 Compare September 17, 2024 09:42

Stavrospanakakis changed the title ~~libcontainer/cgroups/fs: replace strings.Fields with strings.Cut~~ libcontainer/cgroups/fs: remove todo since strings.Fields performs well Sep 17, 2024

Stavrospanakakis requested a review from kolyshkin September 17, 2024 10:10

Stavrospanakakis force-pushed the replace-fields-splitn-cgroups-fs branch from 9a37150 to 4eef116 Compare September 18, 2024 15:42

amghazanfari mentioned this pull request Sep 19, 2024

replace strings.SplitN with strings.Cut #4405

Open

lifubang approved these changes Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libcontainer/cgroups/fs: remove todo since strings.Fields performs well #4403

libcontainer/cgroups/fs: remove todo since strings.Fields performs well #4403

Stavrospanakakis commented Sep 15, 2024 •

edited

Loading

cyphar commented Sep 15, 2024

Stavrospanakakis commented Sep 16, 2024

kolyshkin Sep 16, 2024

Stavrospanakakis Sep 16, 2024

kolyshkin Sep 16, 2024

Stavrospanakakis Sep 16, 2024

kolyshkin commented Sep 16, 2024

kolyshkin commented Sep 16, 2024

Stavrospanakakis commented Sep 16, 2024

kolyshkin Sep 17, 2024

Stavrospanakakis Sep 17, 2024

tianon Sep 18, 2024

kolyshkin Sep 18, 2024

kolyshkin commented Sep 17, 2024

Stavrospanakakis commented Sep 17, 2024

kolyshkin commented Sep 18, 2024

Stavrospanakakis commented Sep 18, 2024

lifubang left a comment

libcontainer/cgroups/fs: remove todo since strings.Fields performs well #4403

Are you sure you want to change the base?

libcontainer/cgroups/fs: remove todo since strings.Fields performs well #4403

Conversation

Stavrospanakakis commented Sep 15, 2024 • edited Loading

Different implementations

strings.Fields

strings.SplitN

strings.Cut

cyphar commented Sep 15, 2024

Stavrospanakakis commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolyshkin commented Sep 16, 2024

kolyshkin commented Sep 16, 2024

Stavrospanakakis commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolyshkin commented Sep 17, 2024

Stavrospanakakis commented Sep 17, 2024

kolyshkin commented Sep 18, 2024

Stavrospanakakis commented Sep 18, 2024

lifubang left a comment

Choose a reason for hiding this comment

Stavrospanakakis commented Sep 15, 2024 •

edited

Loading

`strings.Fields`

`strings.SplitN`

`strings.Cut`