-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
libcontainer/cgroups/fs: remove todo since strings.Fields performs well #4403
libcontainer/cgroups/fs: remove todo since strings.Fields performs well #4403
Conversation
bd59f35
to
a1e7cca
Compare
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...) |
@cyphar I created the 2 functions as Using What do you think? Output:
// 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()
}
} |
libcontainer/cgroups/fs/cpuacct.go
Outdated
fields := strings.Fields(data) | ||
if len(fields) < 4 || fields[0] != userField || fields[2] != systemField { | ||
|
||
lines := strings.SplitN(data, "\n", 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case you can actually use strings.Cut (which was not available when this TODO was written).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a commit that uses strings.Cut
instead of strings.SplitN
and strings.Fields
.
libcontainer/cgroups/fs/cpuacct.go
Outdated
} | ||
|
||
userLine := lines[0] | ||
userLineFields := strings.SplitN(userLine, " ", 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, strings.Cut
will work better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
@Stavrospanakakis do you mind rewriting the benchmark using https://pkg.go.dev/testing#hdr-Benchmarks and making it part of the PR? |
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. |
a1e7cca
to
3518a94
Compare
@kolyshkin I added the benchmark test as a part of this pull request. Also, after running the test,
|
libcontainer/cgroups/fs/cpuacct.go
Outdated
return 0, 0, malformedLine(path, file, data) | ||
} | ||
if userModeUsage, err = strconv.ParseUint(fields[1], 10, 64); err != nil { | ||
|
||
userUsageInTicks = strings.TrimSpace(userUsageInTicks) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there extra spaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, strings.Fields
removes all whitespace around which is ideal for this use case.
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. |
3816b28
to
9a37150
Compare
@kolyshkin I applied these changes to a new commit. |
It would be good to describe in a commit message what we learned here. Something like
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. |
9a37150
to
4eef116
Compare
@kolyshkin Sure, I updated the commit message. I also updated the pull request description to include the benchmark differences and the different implementations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks your work!
4eef116
to
3533eaa
Compare
@lifubang @kolyshkin FYI, I did a |
3533eaa
to
12b622f
Compare
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 <stavrospanakakis@gmail.com>
12b622f
to
1be0676
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Initially, this was a commit to switch from
strings.Fields
tostrings.SplitN
in getCpuUsageBreakdown, sincestrings.Fields
was probably slower than
strings.SplitN
in some old Go versions.Afterwards,
strings.Cut
was also considered for potentialspeed improvements.
After writing a benchmark test, we learned that:
strings.Fields
performance is now adequate;strings.SplitN
is slower thanstrings.Fields
;strings.Cut
had <5% performance gain fromstrings.Fields
;So, remove the TODO and keep the benchmark test.
The following are benchmark differences using
strings.Fields
,strings.SplitN
, andstrings.Cut
.Below, you can find the benchmark differences.
Different implementations
Below, you can find the different implementations.
strings.Fields
strings.SplitN
strings.Cut