istio: improve deep copy for ServiceAttribute #40966
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DeepCopy using reflection is super slow, and a big chunk of init push context time is doing deep copy. This is already improved a lot by this PR: istio#37932 (init push context time drop from 1m30s to 40s). ServiceAttribute DeepCopy is still taking more than 10% cpu time, so improving this function can further reduce the init push context time and hence our propagation delay. Benchmark results: (before) goos: darwin goarch: amd64 pkg: istio.io/istio/pilot/pkg/model cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz BenchmarkServiceDeepCopy BenchmarkServiceDeepCopy-16 132760 8190 ns/op PASS (after) goos: darwin goarch: amd64 pkg: istio.io/istio/pilot/pkg/model cpu: Intel(R) Core(TM) i9-9980HK CPU @ 2.40GHz BenchmarkServiceDeepCopy BenchmarkServiceDeepCopy-16 1213035 1019 ns/op PASS Change-Id: Ied3e81d252ccf226bbb8d1d56eb88bff7c146af4 Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3700 Reviewed-by: Douglas Jordan <douglas.jordan@airbnb.com> Reviewed-by: Weibo He <weibo.he@airbnb.com>
AddressMap contains a mutex which govet complains if we return a copy, ignoring the vet error (behavior is the same as before). Change-Id: If0274e6e1412eb50586ea609a07c302557297ad8 Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3706 Reviewed-by: Weibo He <weibo.he@airbnb.com>
hzxuzhonghu
reviewed
Sep 14, 2022
| @@ -566,7 +566,49 @@ func (s *ServiceAttributes) DeepCopy() ServiceAttributes { | |||
| // Nested mutexes are configured to be ignored by copystructure.Copy. | |||
| // Disabling `go vet` warning since this is actually safe in this case. | |||
| // nolint: vet | |||
| return copyInternal(*s).(ServiceAttributes) | |||
| out := *s | |||
There was a problem hiding this comment.
followed up in #40990
howardjohn
approved these changes
Sep 14, 2022
airbnb-gerrit
pushed a commit
to airbnb/istio
that referenced
this pull request
Oct 4, 2022
This CL patches commit 76f34a3 from upstream istio into air-release-1.14.4 to improve propagation delay. Original PR: istio#40966 Change-Id: Ib208d94a1f84cd0ac496528470928e35103539e8 Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3821 Reviewed-by: Weibo He <weibo.he@airbnb.com>
airbnb-gerrit
pushed a commit
to airbnb/istio
that referenced
this pull request
Oct 12, 2022
Apply the following list of patches to istio 1.14.5: * sidecar: filter service ports to VS ports (istio#39067) * istio: register init push context metric (istio#40049) * istio: add metric for debouncing (istio#40523) * istio: fix PILOT_ENABLE_RDS_CACHE flag not working (istio#40719) * istio: support inline multi-values header in authz header match (https://gerrit.musta.ch/c/public/istio/+/3622, not yet merged upstream) * istio: improve deep copy for ServiceAttribute (istio#40966) * avoid unnecessary copy virtual services for sidecar scope calculation (istio#41101) Change-Id: Ia4c9bfd619a0eb38c1a829bff2efbd21fd3b9cb2 Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3883 Reviewed-by: Ying Zhu <ying.zhu@airbnb.com> Reviewed-by: Weibo He <weibo.he@airbnb.com>
airbnb-gerrit
pushed a commit
to airbnb/istio
that referenced
this pull request
Nov 10, 2022
Apply the following list of upstream commits to istio 1.15.3: * istio: add metric for debouncing (istio#40523) * istio: fix PILOT_ENABLE_RDS_CACHE flag not working (istio#40719) * istio: improve deep copy for ServiceAttribute (istio#40966) * avoid unnecessary copy virtual services for sidecar scope calculation (istio#41101) Change-Id: I2ee1d77d096a329dc8f590151223b37193dd4f1b Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3990 Reviewed-by: Ying Zhu <ying.zhu@airbnb.com> Reviewed-by: Ryan Smick <ryan.smick@airbnb.com>
|
/cherrypick release-1.15 |
|
@S-Chan: new pull request created: #42670 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. |
airbnb-gerrit
pushed a commit
to airbnb/istio
that referenced
this pull request
Feb 10, 2023
Apply the following list of upstream commits to istio 1.15.5: * istio: add metric for debouncing (istio#40523) * istio: improve deep copy for ServiceAttribute (istio#40966) * avoid unnecessary copy virtual services for sidecar scope calculation (istio#41101) Change-Id: I25f31e5633b77982606912bcb2ad2bc4e2da87f4 Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/4381 Reviewed-by: Weibo He <weibo.he@airbnb.com> Reviewed-by: Stephen Chan <stephen.chan@airbnb.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please provide a description of this PR:
our istiod CPU profile shows that 10% of time is spent on DeepCopy of ServiceAttribute, so removing the use of copystructure. We already have fuzz test and benchmark for Service DeepCopy, which covers this part since ServiceAttribute is part of Service.
Benchmark results:
(before)
(after)