Fix excessive allocation in the resource metrics #1964
Conversation
knative-prow-robot
added
the
size/S
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: markusthoemmes 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 |
knative-prow-robot
added
the
approved
Codecov Report
@@ Coverage Diff @@
## master #1964 +/- ##
==========================================
+ Coverage 68.92% 68.94% +0.01%
==========================================
Files 209 209
Lines 8778 8790 +12
==========================================
+ Hits 6050 6060 +10
- Misses 2453 2454 +1
- Partials 275 276 +1
Continue to review full report at Codecov.
|
knative-prow-robot
added
size/M
metrics/resource_view.go
Outdated
| var s strings.Builder | ||
| writeKV := func(key, value string) { | ||
| // We use byte values 1 and 2 to avoid colliding with valid resource labels | ||
| // and to make unpacking easy | ||
| s.WriteByte('\x01') | ||
| s.WriteString(key) | ||
| s.WriteByte('\x02') | ||
| s.WriteString(value) | ||
| } |
There was a problem hiding this comment.
not sure if at this point func writeKV(Builder, string, string) isn't better, or definitely more readable.
Shouldn't it save an allocation, since otherwise it'll dynamically create the lambda to bind it to s?
There was a problem hiding this comment.
The lambda doesn't escape apparently. Moving it out works too though.
|
/lgtm |
Changes
Through a bit of cleanup work in Knative Serving, it got kinda apparent that our metric recording calls are kinda expensive. I triaged it down to the
resourceToKeybeing quite expensive.Discussing with @evankanderson, we came up with this quick fix to make the allocations of the functions much more reasonable without coming up with a different way of handling the keys here altogether.
Benchmark results
/assign @evankanderson