Improve performance by for pid stats (cgroups1) re-using readuint #291
Conversation
manugupt1
force-pushed
the
improve-perf
branch
2 times, most recently
from
b6a34e0
to
bd48e06
Compare
manugupt1
changed the title
| @@ -67,16 +66,10 @@ func (p *pidsController) Stat(path string, stats *v1.Metrics) error { | |||
| if err != nil { | |||
| return err | |||
| } | |||
| var max uint64 | |||
| maxData, err := os.ReadFile(filepath.Join(p.Path(path), "pids.max")) | |||
| max, err := readUint(filepath.Join(p.Path(path), "pids.max")) | |||
There was a problem hiding this comment.
Isn't the behavior different here? readUint should fail if "max" exists in the file as it only knows how to parse integers, so we'd bail on line 71. Before we'd carry on to filling in the stats, granted the max variable will be left as 0 it seems if "max" was in pids.max, but point being this method would still succeed
There was a problem hiding this comment.
Right; I changed readUint to return MaxUint64 now. The behavior is still different but it is consistent with cgroups2. I think this change should be fine now.
There was a problem hiding this comment.
Yea it is odd to me how we returned 0 here before.. would need to see if any users relied on this behavior with a github search maybe..
There was a problem hiding this comment.
(I think returning MaxUint64 makes sense, but.. did anyone rely on this? It being 0 I mean, might wanna stay safe)
There was a problem hiding this comment.
I think it makes sense at least for v3; v1 is imported by so many packages that I am not confident.
- https://pkg.go.dev/github.com/containerd/cgroups?tab=importedby
- https://pkg.go.dev/github.com/containerd/cgroups/v3@v3.0.1?tab=importedby
Thinking of reverting back to 0 and adding a comment to say that we preserve it for backward compatibility.
What do you think?
There was a problem hiding this comment.
I'd err on the side of staying safe, so your reasoning seems sane and I agree (also sorry for the radio silence here 🫠)
There was a problem hiding this comment.
I suppose 0 could be interpreted as no limit, so that might've been the rationale
There was a problem hiding this comment.
Zero wouldn't be a valid value anyway as zero, so that's a fair rationale.
manugupt1
force-pushed
the
improve-perf
branch
6 times, most recently
from
466bd86
to
83e12f7
Compare
Benchstat results
➜ cgroups git:(main) ✗ benchstat old.txt new.txt
goos: linux
goarch: amd64
pkg: github.com/containerd/cgroups/v3/cgroup1
cpu: 11th Gen Intel(R) Core(TM) i7-1165G7 @ 2.80GHz
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
TestPids-8 19.32µ ± 4% 17.39µ ± 2% -9.97% (p=0.000 n=10)
│ old.txt │ new.txt │
│ B/op │ B/op vs base │
TestPids-8 1344.0 ± 0% 624.0 ± 0% -53.57% (p=0.000 n=10)
│ old.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
TestPids-8 13.00 ± 0% 11.00 ± 0% -15.38% (p=0.000 n=10)
➜ cgroups git:(main) ✗
Signed-off-by: Manu Gupta <manugupt1@gmail.com>
| // We should only need 20 bytes for the max uint64, but for a nice power of 2 | ||
| // lets use 32. |
There was a problem hiding this comment.
Per the original comment, is it realistic that this might have leading/trailing whitespace? Seems unlikely, but anyone who does would start breaking if they had a lot of it.
There was a problem hiding this comment.
It seems the files simply have one line feed.
There was a problem hiding this comment.
I guess trailing whitespace isn't actually an issue because it will only read len(b) bytes and stop anyway, so the only way to break this would be to have a bunch of leading whitespace, which seems unlikely in this case.
There was a problem hiding this comment.
Right, it's mostly irrelevant if we only read a fixed size of bytes that will always be larger than a uint64's length. The string gets trimmed and all that's left is what we care about.
cc @kolyshkin @dcantah
I re-used the optimized readuint in cgroups to improve performance. This should not change the behavior for max as readuint returns 0 and when a file is not present; an error is still returned.
PTAL
Benchstat results: