Skip to content
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

Fix Proc.Limits limit name matching #667

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

inkel
Copy link

@inkel inkel commented Sep 19, 2024

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.

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.
@inkel
Copy link
Author

inkel commented Sep 19, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant