pkg/metrics: Tune NewMetrics function #567
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
pkg/metrics/metrics.go
Outdated
| if len(keys) > 0 { | ||
| labels := []string{} | ||
| separator := "{" |
There was a problem hiding this comment.
Just minor, but you could write separater := '{' here and use WriteByte below.
pkg/metrics/metrics.go
Outdated
|
|
||
| return "" | ||
| m.WriteString("}") |
There was a problem hiding this comment.
m.writeByte('}') is fundamentally easier.
pkg/metrics/metrics.go
Outdated
| m.WriteString(keys[i]) | ||
| m.WriteString("=\"") | ||
| escapeString(m, values[i]) | ||
| m.WriteString("\"") |
There was a problem hiding this comment.
m.writeByte('"') is fundamentally easier.
pkg/metrics/metrics.go
Outdated
| m.WriteString("=\"") | ||
| escapeString(m, values[i]) | ||
| m.WriteString("\"") | ||
| separator = "," |
pkg/metrics/metrics.go
Outdated
| @@ -77,8 +91,34 @@ var ( | |||
| // escapeString replaces '\' by '\\', new line character by '\n', and - if | |||
| // includeDoubleQuote is true - '"' by '\"'. | |||
There was a problem hiding this comment.
This doc comment is not quite correct.
- Use strings.Builder minimizing memory allocations. - Use strconv to render float64 in combination with a []byte buffer pool instead of fmt.Sprintf reducing memory allocations. - Only sort labels of metrics in test framework, not by default (e.g. in production). Benchmark patch: ```bash go get golang.org/x/tools/cmd/benchcmp git checkout perf-exp~1 go test -v -bench=NewMetric -run=^$ -benchmem ./pkg/metrics/... > old.txt git checkout perf-exp go test -v -bench=NewMetric -run=^$ -benchmem ./pkg/metrics/... > new.txt benchcmp old.txt new.txt benchmark old ns/op new ns/op delta BenchmarkNewMetric/value-1-4 2652 612 -76.92% BenchmarkNewMetric/value-35.7-4 2706 755 -72.10% benchmark old allocs new allocs delta BenchmarkNewMetric/value-1-4 32 6 -81.25% BenchmarkNewMetric/value-35.7-4 32 6 -81.25% benchmark old bytes new bytes delta BenchmarkNewMetric/value-1-4 1616 528 -67.33% BenchmarkNewMetric/value-35.7-4 1616 528 -67.33% ```
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brancz, mxinden The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
|
||
| m = m + "\n" | ||
| m.WriteString("\n") |
There was a problem hiding this comment.
DIdn't catch this in my first round of review: This could be written as
m.WriteByte('\n')There was a problem hiding this comment.
Right, sorry about that. Follow up: #572
What this PR does / why we need it:
This patch tunes the
NewMetricsfunction based on optimizations discussed in prometheus/common#148 and #498 (comment) (//CC @beorn7).Use strings.Builder minimizing memory allocations.
Use strconv to render float64 in combination with a []byte buffer pool
instead of fmt.Sprintf reducing memory allocations.
Only sort labels of metrics in test framework, not by default (e.g. in
production).
Benchmark patch: