Skip to content

Updating EndpointSliceCache sort function to be significantly faster.#83035

Merged
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
robscott:endpointslice-proxy-sort-perf
Sep 25, 2019
Merged

Updating EndpointSliceCache sort function to be significantly faster.#83035
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
robscott:endpointslice-proxy-sort-perf

Conversation

@robscott
Copy link
Copy Markdown
Member

@robscott robscott commented Sep 24, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
The .IP() call that was previously used for sorting resulted in a call to netutil to parse an IP out of an IP:Port string. This was very slow and resulted in this sort taking up ~50% of total CPU util for kube-proxy.

Does this PR introduce a user-facing change?:

Improved performance of kube-proxy with EndpointSlice enabled with more efficient sorting. 

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/sig network
/priority important-soon
/cc @freehan @bowei

The .IP() call that was previously used for sorting resulted in a call
to netutil to parse an IP out of an IP:Port string. This was very slow
and resulted in this sort taking up ~50% of total CPU util for
kube-proxy.
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/network Categorizes an issue or PR as relevant to SIG Network. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 24, 2019
@robscott
Copy link
Copy Markdown
Member Author

/retest

}
func (e byIP) Less(i, j int) bool {
return e[i].IP() < e[j].IP()
return e[i].String() < e[j].String()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add CompareByIP(Endpoint* other) int to the Endpoint interface and implement with bytes.compare() to avoid allocating a string on each comparison.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bowei This actually is just returning an IP:Port string that happens to be the only string stored with this endpoint struct. Previously when using the .IP() method it was actually resulting in a netutil call to parse that out which was resulting in the slowness. Here it's just returning the string that is already stored as is.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, I thought for some reason it was stored as an IP internally.

@bowei
Copy link
Copy Markdown
Member

bowei commented Sep 24, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 24, 2019
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, robscott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/network Categorizes an issue or PR as relevant to SIG Network. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants