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.

@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.

@discordianfish
Copy link
Member

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.

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.

Signed-off-by: Leandro López (inkel) <[email protected]>
@inkel inkel force-pushed the inkel/proc_limits/fix-re-test branch from 11be0ac to 289d012 Compare September 25, 2024 14:18
@inkel
Copy link
Author

inkel commented Sep 25, 2024

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.

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.

3 participants