Accept protobuf encoded messages in the statserver.#8626
Accept protobuf encoded messages in the statserver.#8626knative-prow-robot merged 4 commits intoknative:masterfrom
Conversation
This further improves the efficiency of the statserver by allowing it to receive a slice of StatMessages, binary-encoded via protobufs. The protobufs are already quite an improvement over using JSON, both in terms of density and serdes performance. Collapsing all stats into a single message further reduces the overhead of the surrounding protocols, making for a much more efficient protocol overall.
knative-prow-robot
added
the
size/L
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
approved
knative-prow-robot
left a comment
knative-prow-robot
left a comment
There was a problem hiding this comment.
@markusthoemmes: 0 warnings.
Details
In response to this:
Proposed Changes
This further improves the efficiency of the statserver by allowing it to receive a slice of StatMessages, binary-encoded via protobufs. The protobufs are already quite an improvement over using JSON, both in terms of density and serdes performance. Collapsing all stats into a single message further reduces the overhead of the surrounding protocols, making for a much more efficient protocol overall.
Benchmark results
goos: linux goarch: amd64 pkg: knative.dev/serving/pkg/autoscaler/statserver BenchmarkStatServer/json-encoding-1-msgs-16 37719 32458 ns/op 3661 B/op 23 allocs/op BenchmarkStatServer/proto-encoding-1-msgs-16 53277 22843 ns/op 2600 B/op 15 allocs/op BenchmarkStatServer/json-encoding-2-msgs-16 23275 50602 ns/op 7322 B/op 46 allocs/op BenchmarkStatServer/proto-encoding-2-msgs-16 50244 24294 ns/op 3088 B/op 24 allocs/op BenchmarkStatServer/json-encoding-5-msgs-16 14947 83316 ns/op 18308 B/op 115 allocs/op BenchmarkStatServer/proto-encoding-5-msgs-16 43286 28117 ns/op 4624 B/op 50 allocs/op BenchmarkStatServer/json-encoding-10-msgs-16 9296 130637 ns/op 36618 B/op 230 allocs/op BenchmarkStatServer/proto-encoding-10-msgs-16 42984 28248 ns/op 7136 B/op 91 allocs/op BenchmarkStatServer/json-encoding-20-msgs-16 5286 228309 ns/op 73225 B/op 460 allocs/op BenchmarkStatServer/proto-encoding-20-msgs-16 28622 41278 ns/op 16272 B/op 173 allocs/op BenchmarkStatServer/json-encoding-50-msgs-16 2389 490206 ns/op 183048 B/op 1151 allocs/op BenchmarkStatServer/proto-encoding-50-msgs-16 16401 72992 ns/op 39280 B/op 415 allocs/op BenchmarkStatServer/json-encoding-100-msgs-16 1308 938296 ns/op 366103 B/op 2302 allocs/op BenchmarkStatServer/proto-encoding-100-msgs-16 10000 121983 ns/op 63856 B/op 816 allocs/op PASS ok knative.dev/serving/pkg/autoscaler/statserver 21.040sRelease Note
NONE/assign @vagababov @julz
Instructions 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/test-infra repository.
julz
left a comment
julz
left a comment
There was a problem hiding this comment.
couple of questions but only really holding off lgtm for the tiny tiny nit and to let @vagababov look
| switch messageType { | ||
| case websocket.BinaryMessage: | ||
| dec = gob.NewDecoder(bytes.NewBuffer(msg)) | ||
| var wsms metrics.WireStatMessages |
There was a problem hiding this comment.
kind of a shame we can't reuse this variable between iterations, but I don't know if it even matters since it's on the stack anyway. probably not 🤷.
There was a problem hiding this comment.
I think unmarshal call escapes it anyway. But there is Reset standard proto method, so it can be reused, but I am not sure if is helpful from performance standpoint.
vagababov
left a comment
vagababov
left a comment
There was a problem hiding this comment.
/lgtm
/hold
If you want to fix the nits
| switch messageType { | ||
| case websocket.BinaryMessage: | ||
| dec = gob.NewDecoder(bytes.NewBuffer(msg)) | ||
| var wsms metrics.WireStatMessages |
There was a problem hiding this comment.
I think unmarshal call escapes it anyway. But there is Reset standard proto method, so it can be reused, but I am not sure if is helpful from performance standpoint.
knative-prow-robot
added
do-not-merge/hold
|
The following is the coverage report on the affected files.
|
|
@markusthoemmes: The following test failed, say
Full PR test history. Your PR dashboard. 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/test-infra repository. I understand the commands that are listed here. |
|
/unhold |
knative-prow-robot
removed
the
do-not-merge/hold
6 participants
Proposed Changes
This further improves the efficiency of the statserver by allowing it to receive a slice of StatMessages, binary-encoded via protobufs. The protobufs are already quite an improvement over using JSON, both in terms of density and serdes performance. Collapsing all stats into a single message further reduces the overhead of the surrounding protocols, making for a much more efficient protocol overall.
Benchmark results
Release Note
/assign @vagababov @julz