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

Add ProcSMaps from smaps file #281

Closed
wants to merge 5 commits into from
Closed

Conversation

PierreF
Copy link
Contributor

@PierreF PierreF commented Apr 7, 2020

The smaps file contains information not available though existing source.

The one that interest me here is the "Pss" and "SwapPss" which is the process proportional usage of resident memory and swap respectively (which is I think the pages size divided by the number of process that share those pages).

I need this to expose a more significant memory usage for a group of process for prometheus process-exporter (ncabatoff/process-exporter#140)

Copy link
Collaborator

@pgier pgier left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Overall looks good, just had a few minor suggestions inline.

proc_smaps.go Outdated
@@ -0,0 +1,246 @@
// Copyright 2018 The Prometheus Authors
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be 2020, maybe someday we'll set up some automation for these notices.

proc_smaps.go Outdated
return
}

// SharedCleanSum returns the sum of Pss from all mappings
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like a copy paste typo on these comments since they all say Pss. Maybe just set the comments to ...sum of all mappings since it should be clear anyway which field is being summed.

line := scan.Text()
// first line of a mapping start with an hexadecimal address in lower-case
// All other line start with a capitalized words.
if (line[0] >= '0' && line[0] <= '9') || (line[0] >= 'a' && line[0] <= 'f') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be more clear hear to do a simple regex match.

Copy link
Contributor Author

@PierreF PierreF May 1, 2020

Choose a reason for hiding this comment

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

I've kept is as an if condition for performance reason.
I've added a benchmark and pushed PierreF@d9c39b8 which use regexp, but it has notable performance impact:

$ git co ea36d86 # commit just before using regexp
$ for i in `seq 0 10`; do go test -run=none -bench=.* ./ ;done > bench.ea36d86
$ git co d9c39b8 # commit using regexp
$ for i in `seq 0 10`; do go test -run=none -bench=.* ./ ;done > bench.d9c39b8
$ go get golang.org/x/perf/cmd/benchstat
$ benchstat bench.ea36d86 bench.d9c39b8 
name         old time/op  new time/op  delta
ProcSmaps-8   199µs ±13%   266µs ± 2%  +33.87%  (p=0.000 n=11+9)

I think this the less clear hand-made condition worse the performance gain, but the decision is yours.

proc_smaps.go Outdated
v := strings.TrimSpace(kv[1])
v = strings.TrimRight(v, " kB")

vKBytes, _ := strconv.ParseUint(v, 10, 64)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be better to check this error and fail if it's non-nil unless there is reason to ignore the parse failures.

@@ -0,0 +1,74 @@
// Copyright 2018 The Prometheus Authors
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be 2020.

PierreF added 5 commits May 1, 2020 13:30
Signed-off-by: Pierre Fersing <[email protected]>
Signed-off-by: Pierre Fersing <[email protected]>
Signed-off-by: Pierre Fersing <[email protected]>
Signed-off-by: Pierre Fersing <[email protected]>
@PierreF
Copy link
Contributor Author

PierreF commented May 1, 2020

Fixed comments. About the regexp, I've kept the if condition for performance reason. If you want to go the the regexp version, I'll update this PR to include the commit (PierreF@d9c39b8)

@SuperQ
Copy link
Member

SuperQ commented May 1, 2020

FYI, we might want to use smaps_rolloup rather than smaps. It's a lighter-weight interface to the PSS data. But requires Linux >= 4.15.x

@PierreF
Copy link
Contributor Author

PierreF commented May 1, 2020

This looks very interesting... and exactly match what I need for ncabatoff/process-exporter#140.
I'll propose a new PR based on this new file, with fallback on smaps, which will only expose the rollup.

@PierreF
Copy link
Contributor Author

PierreF commented May 1, 2020

Created #286 which use smaps_rollup. I think #286 is better: it's faster (on the benchmark, and likely even more in real usage since the Kernel don't need to provide detailed information) and should provide enough information for most usage.

@discordianfish
Copy link
Member

Ok lets close this in favor of #286 for now.

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.

4 participants