Skip to content

feat: replace custom sysctl implementation with stdlib #135

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

Merged
merged 7 commits into from
Sep 15, 2022

Conversation

kruskall
Copy link
Member

@kruskall kruskall commented Sep 12, 2022

Remove custom string->mib conversion code and sysctl implementation.
Reduce code complexity and cgo dependency

Related to elastic/apm-server#8718

Remove custom string->mib conversion code and sysctl
implementation.
Reduce code complexity and cgo dependency
@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 12, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-09-14T17:28:16.307+0000

  • Duration: 8 min 30 sec

Test stats 🧪

Test Results
Failed 0
Passed 98
Skipped 2
Total 100

@andrewkroh andrewkroh self-requested a review September 14, 2022 14:09
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The one concern I had was with performance. The only difference is that the unix package makes two sysctl calls (one to determine buf size and one to get the data). The old implementation in this package made one call with a max size buffer.

This does make it a bit slower, but it's not significant.

func BenchmarkKernProcargs(b *testing.B) {
	pid := os.Getpid()
	var p process
	for i := 0; i < b.N; i++ {
		kern_procargs(pid, &p)
	}
}
% benchcmp before-self.txt after-self.txt                                                      
benchcmp is deprecated in favor of benchstat: https://pkg.go.dev/golang.org/x/perf/cmd/benchstat
benchmark                    old ns/op     new ns/op     delta
BenchmarkKernProcargs-10     13783         15075         +9.37%

benchmark                    old allocs     new allocs     delta
BenchmarkKernProcargs-10     106            109            +2.83%

benchmark                    old bytes     new bytes     delta
BenchmarkKernProcargs-10     31931         34104         +6.81%
% benchstat before-self.txt after-self.txt
name             old time/op    new time/op    delta
KernProcargs-10    13.8µs ± 0%    15.1µs ± 0%   ~     (p=1.000 n=1+1)

name             old alloc/op   new alloc/op   delta
KernProcargs-10    31.9kB ± 0%    34.1kB ± 0%   ~     (p=1.000 n=1+1)

name             old allocs/op  new allocs/op  delta
KernProcargs-10       106 ± 0%       109 ± 0%   ~     (p=1.000 n=1+1)

@andrewkroh
Copy link
Member

And can you please add changelog entries for this and the other recent changes.

@kruskall kruskall requested a review from andrewkroh September 14, 2022 15:36
@kruskall
Copy link
Member Author

Removed unused methods and added the changelog entry 👍

@kruskall kruskall changed the base branch from main to bugfix/oraclelinux-redhat-family September 14, 2022 23:33
@kruskall kruskall changed the base branch from bugfix/oraclelinux-redhat-family to main September 14, 2022 23:33
@kruskall kruskall closed this Sep 15, 2022
@kruskall kruskall reopened this Sep 15, 2022
@kruskall kruskall merged commit 15f096c into elastic:main Sep 15, 2022
@kruskall kruskall deleted the feat/replace-custom-sysctl branch September 15, 2022 15:55
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