-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
optimize PatchResponseFromRaw #2432
Conversation
|
Welcome @d3c3mber! |
Hi @d3c3mber. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: d3c3mber The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
BenchmarkHandle/benchmark_handle_function | ||
BenchmarkHandle/benchmark_handle_function-10 44620 26178 ns/op | ||
*/ | ||
func BenchmarkHandle(b *testing.B) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two benchmarks are not representative, as you are only benchmarking the "objects are equal case" and not the "objects are not equal", in which case your change worsens performance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your prompt reply @alvaroaleman
I've add more benchmark tests regarding equal case and not equal case.
BenchmarkPatchResponseFromRawAndIsByteArrayEqual/benchmark_PatchResponseFromRaw_function-10 52774 22749 ns/op
BenchmarkPatchResponseFromRawAndIsByteArrayEqual/benchmark_isByteArrayEqual_function-10 5252599 230.7 ns/op
To add more details to this:
- By benchmarking, comparing two byte arrays takes far less time than the recursive reflect.DeepEqual, which is about 1/100 according to my above benchmark
- Most of the Apiserver's patch/update requests will still get same object after going through all mutating webhooks. as mutating webhooks most of the time take effect at the beginning of a Pod's lifecycle (or other resources lifecycle)
- In our production cluster, this change "reducing recursive reflect.DeepEqual" can save more than 40% cpu time for webhook.
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
@@ -86,6 +86,17 @@ func ValidationResponse(allowed bool, message string) Response { | |||
// The original object should be passed in as raw bytes to avoid the roundtripping problem | |||
// described in https://github.com/kubernetes-sigs/kubebuilder/issues/510. | |||
func PatchResponseFromRaw(original, current []byte) Response { | |||
if isByteArrayEqual(original, current) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if isByteArrayEqual(original, current) { | |
if bytes.Equal(original, current) { |
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. In response to this:
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. |
fixes #2431