procfs icon indicating copy to clipboard operation
procfs copied to clipboard

Fix Proc.Limits limit name matching

Open inkel opened this issue 1 year ago • 3 comments

I was working on improving this algorithm to reduce the number of allocations when I found out that with the addition of the additional test cases, Max processes was failing to match the switch statement as for some reason the limit name has a trailing whitespace. By trimming the spaces it now matches all cases.

I've also locally tested with changing the values of the fixture file for the rest of the limits and it was matching in all cases.

inkel avatar Sep 19 '24 18:09 inkel

I've also been working on adding a benchmark for this function, in its current form I've got these results:

$ go test -run=TestLimits -bench=BenchmarkLimits -benchmem
goos: darwin
goarch: arm64
pkg: github.com/prometheus/procfs
cpu: Apple M1 Pro
BenchmarkLimits-10         49003             23055 ns/op            7704 B/op         56 allocs/op
PASS
ok      github.com/prometheus/procfs    1.602s

I've been trying to add an algorithm that parses the limits file line to get the values, and it currently passes all tests and have the following results:

$ go test -run=TestLimits -bench=BenchmarkLimits -benchmem
goos: darwin
goarch: arm64
pkg: github.com/prometheus/procfs
cpu: Apple M1 Pro
BenchmarkLimits-10        104811             11415 ns/op            6621 B/op         40 allocs/op
PASS
ok      github.com/prometheus/procfs    1.883s

Comparing using benchstat gives the following:

goos: darwin
goarch: arm64
pkg: github.com/prometheus/procfs
cpu: Apple M1 Pro
          │   re.log    │              inkel.log              │
          │   sec/op    │   sec/op     vs base                │
Limits-10   23.25µ ± 4%   11.45µ ± 5%  -50.76% (p=0.000 n=10)

          │    re.log    │              inkel.log               │
          │     B/op     │     B/op      vs base                │
Limits-10   7.520Ki ± 0%   6.466Ki ± 0%  -14.01% (p=0.000 n=10)

          │   re.log   │             inkel.log              │
          │ allocs/op  │ allocs/op   vs base                │
Limits-10   56.00 ± 0%   40.00 ± 0%  -28.57% (p=0.000 n=10)

Currently the algorithm is quite awful, but I'm planning on improving it to make it more readable. If you think it's worth the effort, I could try and create a new pull request if needed.

inkel avatar Sep 19 '24 18:09 inkel

Kinda odd for the limit name having a trailing whitespace.. Any idea why that might happen? But not opposed to the change.. As for the performance improvement, feel free to submit a separate PR for that. You also need to sign the commit, see failing check.

discordianfish avatar Sep 25 '24 06:09 discordianfish

Thank you @discordianfish, I just pushed a signed-off commit.

As for why it's happening I also found it odd but couldn't figure it out yet.

inkel avatar Sep 25 '24 14:09 inkel

Thank you @SuperQ!

inkel avatar Oct 21 '25 12:10 inkel