-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
prepare selector compute #39597
prepare selector compute #39597
Conversation
Signed-off-by: zounengren <zouyee1989@gmail.com>
😊 Welcome @zouyee! This is either your first contribution to the Istio istio repo, or it's been You can learn more about the Istio working groups, code of conduct, and contributing guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
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.
instead of catching the selector would it be simpler to just re-implement the selector using the map directly? It should be fairly simple and avoid the cost.
A bit worried about complexity here.
This method has been well practiced in the endpont controller of k8s. |
Signed-off-by: zounengren <zouyee1989@gmail.com>
I don't think that is a convincing argument to do this on its own. Is there a technical reason this complexity is justified compared to just using the map directly? |
Not sure have you tested the performance? Which function call is CPU costing? |
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.
/hold
// ServiceSelectorCache is a cache of service selectors to avoid high CPU consumption caused by frequent calls to AsSelectorPreValidated (see #73527) | ||
type ServiceSelectorCache struct { | ||
lock sync.RWMutex | ||
cache map[string]labels.Selector |
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.
This is heavily increasing memory cost
original
current GetPodServiceMemberships which use ServiceSelectorCache
more than 50% performance improvement |
You have to weigh off cpu and memory. It is not justice to say only CPU |
Please also compare it to just using the map directly. |
@zouyee: The following test failed, say
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. I understand the commands that are listed here. |
🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2022-06-29. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions. Created by the issue and PR lifecycle manager. |
Signed-off-by: zounengren zouyee1989@gmail.com
Please provide a description of this PR:
ServiceSelectorCache is a cache of service selectors to avoid high CPU consumption caused by frequent calls to AsSelectorPreValidated