client-go: add a benchmark for the watch event decode path#140930
client-go: add a benchmark for the watch event decode path#140930ashishpatel26 wants to merge 1 commit into
Conversation
Issue kubernetes#129705 reports that StreamWatcher's decode path dominates kube-apiserver memory under high pod churn, backed by heap profiles from a large cluster. There was no in-tree benchmark for that path, so the cost could not be measured or regression-tested locally. I added one that mirrors the stack rest.Request.newStreamWatcher builds for a protobuf watch: length-delimited framer -> streaming decoder -> restclientwatch.Decoder, decoding a realistically-shaped Pod event. On my machine the current cost is 4241 B/op and 33 allocs/op per event. This only adds the benchmark; it does not change decode behavior. My findings on why the buffer-reuse direction suggested on the issue does not help are in a comment there.
|
Please note that we're already in Code Freeze for the upcoming v1.37.0 release. Adding the milestone to this PR is strictly prohibited without proper approval. If this PR needs to be included in the v1.37.0 release:
|
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Hi @ashishpatel26. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ashishpatel26 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/ok-to-test |
|
/retest |
|
@ashishpatel26: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
What this PR does / why we need it
#129705 reports that the client-side watch decode path dominates kube-apiserver memory under high pod churn, backed by heap profiles from a large cluster. There was no in-tree benchmark covering that path, so the cost couldn't be measured locally or protected against regression.
I added one. It mirrors the exact stack
rest.Request.newStreamWatcherbuilds for a protobuf watch - length-delimited framer ->streaming.Decoder->restclientwatch.Decoder- decoding a realistically-shaped Pod event.Current cost on my machine:
This PR only adds the benchmark. It does not change decode behavior.
Note on the fix direction discussed in the issue
While writing this I looked into the "single buffer per watcher for deserializing data" idea from the issue thread, and I don't think it works at this layer. Recording what I found so nobody repeats it:
The frame-read buffer is already reused per watcher -
streaming.decoderkeeps a singled.bufand grows it in place (streaming.go). So that part is done.Reusing the per-event
metav1.WatchEventinDecoder.Decode()doesn't help either, because both protobuf decode paths callunmarshaler.Reset()before unmarshalling, andWatchEvent.Reset()is*m = WatchEvent{}- which drops theObject.Rawbacking array every time, so its capacity can never be recycled.I measured that rather than assuming it: reusing the struct saves exactly 1 allocation out of 33 (4241 -> 4176 B/op, ~1.5%), with no ns/op improvement. That's not worth adding hidden state to a path every informer in every client depends on, so I dropped it and kept only the benchmark.
The remaining 32 allocations are inside the embedded object unmarshal itself - materializing the Pod's maps, slices and strings. Avoiding those would mean aliasing the frame buffer into decoded objects, which isn't safe here:
d.bufis overwritten on the next frame read, so anything retained by an informer cache would be corrupted. A real fix looks like it needs a change to the serializer'sResetcontract, which is a cross-cutting design decision I'd want a sig-api-machinery opinion on before attempting.Which issue(s) this PR is related to
Relates to #129705 (adds measurement; does not fix)
Type of change
/kind cleanup
/sig api-machinery
/sig scalability
Does this PR introduce a user-facing change?
Testing
go vet ./rest/watch/- cleango test ./rest/watch/- all existing tests passgo test ./rest/watch/ -run XXX -bench BenchmarkDecoderProtobuf- runs and reports stable allocation numbers across repeated runsTest-only addition, so there's no production code path to regress.