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
cleanup: Refactor BaseEndpointInfo to cache IP and Port values #120104
Conversation
Hi @togettoyou. Thanks for your PR. I'm waiting for a kubernetes 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. |
pkg/proxy/iptables/proxier.go
Outdated
@@ -132,7 +132,7 @@ type endpointsInfo struct { | |||
func newEndpointInfo(baseInfo *proxy.BaseEndpointInfo, svcPortName *proxy.ServicePortName) proxy.Endpoint { | |||
return &endpointsInfo{ | |||
BaseEndpointInfo: baseInfo, | |||
ChainName: servicePortEndpointChainName(svcPortName.String(), strings.ToLower(string(svcPortName.Protocol)), baseInfo.Endpoint), | |||
ChainName: servicePortEndpointChainName(svcPortName.String(), strings.ToLower(string(svcPortName.Protocol)), baseInfo.Endpoint.String()), |
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.
I have doubts this improves performance, since we need to concatenate IP port in each of these calls
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.
Yes, currently the endpointString
struct only stores the IP and Port, so the proxier
doesn't need to perform additional parsing to extract them. However, for the String
method, concatenation is needed.
I can add an endpoint field to the endpointString struct:
type endpointString struct {
IP string
Port int
endpoint string
}
func (e *endpointString) String() string {
if e.endpoint == "" {
e.endpoint = net.JoinHostPort(e.IP, strconv.Itoa(e.Port))
}
return e.endpoint
}
What do you think?
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.
I just saw you claimed the performance improvement, and I was wondering how we can know if we regress or improve, without that information is hard to evaluate if this change is necessary
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.
https://github.com/kubernetes/kubernetes/blob/master/pkg/proxy/iptables/proxier.go#L1371
https://github.com/kubernetes/kubernetes/blob/master/pkg/proxy/ipvs/proxier.go#L1051
At least here, there is no need to repeatedly extract the Endpoint string when calling the IP()
and Port()
methods.
Do you have any suggestions for performance improvement testing?
When the func BenchmarkEndpointIP(b *testing.B) {
info := &BaseEndpointInfo{
Endpoint: "1.1.1.1:11",
}
for i := 0; i < b.N; i++ {
_ = info.IP()
}
}
func BenchmarkEndpointPort(b *testing.B) {
info := &BaseEndpointInfo{
Endpoint: "1.1.1.1:11",
}
for i := 0; i < b.N; i++ {
_, _ = info.Port()
}
}
func BenchmarkEndpointString(b *testing.B) {
info := &BaseEndpointInfo{
Endpoint: "1.1.1.1:11",
}
for i := 0; i < b.N; i++ {
_ = info.String()
}
}
When the func BenchmarkEndpointIP(b *testing.B) {
info := &BaseEndpointInfo{
Endpoint: endpointString{
IP: "1.1.1.1",
Port: 11,
},
}
for i := 0; i < b.N; i++ {
_ = info.IP()
}
}
func BenchmarkEndpointPort(b *testing.B) {
info := &BaseEndpointInfo{
Endpoint: endpointString{
IP: "1.1.1.1",
Port: 11,
},
}
for i := 0; i < b.N; i++ {
_, _ = info.Port()
}
}
func BenchmarkEndpointString(b *testing.B) {
info := &BaseEndpointInfo{
Endpoint: endpointString{
IP: "1.1.1.1",
Port: 11,
},
}
for i := 0; i < b.N; i++ {
_ = info.String()
}
}
|
Hey @aojea , Can this be used as an evaluation method? |
pkg/proxy/endpoints.go
Outdated
endpoint string | ||
} | ||
|
||
func NewEndpointString(s string) endpointString { |
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.
Currently, the NewEndpointString
method is mainly used in *_test.go
files, and I am not sure if it needs to return an err
ok, so both epInfo.IP( )and epInfo.String() are called in the same loop, so we can assume both are going to be called the same number of times kubernetes/pkg/proxy/iptables/proxier.go Lines 1353 to 1378 in fb785f1
The benchmarks shows something that is expected, concatenation is faster than splitting
There is one solution for these problems, that is sacrificing memory to cache the value instead of generating it each time I do not know if we need to complicate this further creating a new type, and just adding the values to the diff --git a/pkg/proxy/endpoints.go b/pkg/proxy/endpoints.go
index fd1e9485cdb..5ab519ca988 100644
--- a/pkg/proxy/endpoints.go
+++ b/pkg/proxy/endpoints.go
@@ -30,7 +30,6 @@ import (
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/kubernetes/pkg/proxy/metrics"
- proxyutil "k8s.io/kubernetes/pkg/proxy/util"
)
var supportedEndpointSliceAddressTypes = sets.New[string](
@@ -43,7 +42,11 @@ var supportedEndpointSliceAddressTypes = sets.New[string](
// or can be used for constructing a more specific EndpointInfo struct
// defined by the proxier if needed.
type BaseEndpointInfo struct {
- Endpoint string // TODO: should be an endpointString type
+ // Cache this values to improve performance
+ ip string
+ port int
+ // Endpoint is the same as net.JoinHostPort(ip,port)
+ Endpoint string
// IsLocal indicates whether the endpoint is running in same host as kube-proxy.
IsLocal bool
@@ -109,12 +112,12 @@ func (info *BaseEndpointInfo) GetZoneHints() sets.Set[string] {
// IP returns just the IP part of the endpoint, it's a part of proxy.Endpoint interface.
func (info *BaseEndpointInfo) IP() string {
- return proxyutil.IPPart(info.Endpoint)
+ return info.ip
}
// Port returns just the Port part of the endpoint.
func (info *BaseEndpointInfo) Port() (int, error) {
- return proxyutil.PortPart(info.Endpoint)
+ return info.port, nil
}
// Equal is part of proxy.Endpoint interface.
@@ -137,6 +140,8 @@ func (info *BaseEndpointInfo) GetZone() string {
func newBaseEndpointInfo(IP, nodeName, zone string, port int, isLocal bool,
ready, serving, terminating bool, zoneHints sets.Set[string]) *BaseEndpointInfo {
return &BaseEndpointInfo{
+ ip: IP,
+ port: port,
Endpoint: net.JoinHostPort(IP, strconv.Itoa(port)),
IsLocal: isLocal,
Ready: ready, |
Thank you @aojea , It seems that your changes have the smallest impact, but I'm not sure which one is better. Does anyone else have any suggestions? Also, I found that before I added the But after adding it in this commit (0891fe2), it stopped working. I tried your version, but the same problem occurred. kubernetes/pkg/proxy/endpoints_test.go Lines 1592 to 1609 in fb785f1
What could be the reason for this? You can try your modified version as well, it should have the same problem. |
I have found the issue that caused the unit test to fail. kubernetes/pkg/proxy/endpoints_test.go Line 1634 in fb785f1
It was because kubernetes/pkg/proxy/endpointslicecache.go Lines 315 to 316 in fb785f1
Similarly, in your version, the issue was caused by kubernetes/pkg/proxy/endpoints_test.go Lines 1673 to 1674 in fb785f1
I will fix my PR to ensure that the unit test can pass successfully. |
Hi @aojea , I have made a modification to the The Otherwise, many places would require importing the |
f38f391
to
38931e8
Compare
Agreed. The type doesn't add much. |
pkg/proxy/endpoints.go
Outdated
} | ||
|
||
// Port returns just the Port part of the endpoint. | ||
func (info *BaseEndpointInfo) Port() (int, error) { | ||
return proxyutil.PortPart(info.Endpoint) | ||
return info.Endpoint.port, nil | ||
} |
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.
there's no need for this to still return an error, since it can't fail
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 will require modifying the Endpoint
interface
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.
now it should still return an err
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 will require modifying the
Endpoint
interface
That's fine; these are all internal APIs. You can fix all of the consumers as well.
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 will require modifying the
Endpoint
interfaceThat's fine; these are all internal APIs. You can fix all of the consumers as well.
I have resubmitted it and now I need to keep returning err.
pkg/proxy/topology_test.go
Outdated
&BaseEndpointInfo{Endpoint: "10.1.2.4:80", ZoneHints: sets.New[string]("zone-b"), Ready: true}, | ||
&BaseEndpointInfo{Endpoint: "10.1.2.5:80", ZoneHints: sets.New[string]("zone-c"), Ready: true}, | ||
&BaseEndpointInfo{Endpoint: "10.1.2.6:80", ZoneHints: sets.New[string]("zone-a"), Ready: true}, | ||
&BaseEndpointInfo{Endpoint: NewEndpointString("10.1.2.3:80"), ZoneHints: sets.New[string]("zone-a"), Ready: true}, |
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 can use newBaseEndpointInfo
(or else some new helper function just in this file)
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.
I have made the modifications, and the changes are minimal, ensuring full compatibility with the previous unit tests.
ok, #121097 is merged. You should be able to rebase on top of that now... |
7690e6f
to
84a2633
Compare
ready ok-to-test |
pkg/proxy/winkernel/proxier.go
Outdated
if err != nil { | ||
portNumber = 0 | ||
} | ||
portNumber := baseInfo.Port() |
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.
you could just do this inline below, now that you don't need to deal with an error (like it does with ip: baseInfo.IP()
)
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.
yes, I overlooked it.
/ok-to-test Looks good. Sorry for getting your nice simple improvement stuck behind my big refactoring, but I think the end result is better. |
LGTM label has been added. Git tree hash: cd709d67fb76c8a13f10f4253bf91050fada0ec4
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, togettoyou The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
84a2633
to
7a91051
Compare
/retest |
/lgtm |
LGTM label has been added. Git tree hash: db403040068ab6167b01cbbd62dd4154622f1ad2
|
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This commit refactors the
BaseEndpointInfo
struct to add two new fields:ip
andport
. These fields are used to cache the IP and Port values to improve performance.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: