-
Notifications
You must be signed in to change notification settings - Fork 39.2k
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
Improve DeltaFIFO function 'ListKeys' #104725
Conversation
Welcome @NoicFank! |
Hi @NoicFank. 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. |
list := make([]string, 0, len(f.items)) | ||
for key := range f.items { | ||
list := make([]string, 0, len(f.queue)) | ||
for _, key := range f.queue { | ||
list = append(list, key) |
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.
Maybe we can use copy func to complete this work.
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.
The time consumption between using copy func and current method are nearly same.
benchmark archelurong@ARCHELURONG-MB0 ~/go/src/test1 go run main.go 10000
Average time per 10000 times
using queue: 83.75µs
using items: 1128.9µs
Improving: 13.479402985074627
archelurong@ARCHELURONG-MB0 ~/go/src/test1 go run main.go 10000
Average time per 10000 times
using queue: 88.09µs
using items: 1103.31µs
Improving: 12.524804177545692
archelurong@ARCHELURONG-MB0 ~/go/src/test1 go run main.go 10000
Average time per 10000 times
using queue: 85.78µs
using items: 1145.68µs
Improving: 13.356027045931453
archelurong@ARCHELURONG-MB0 ~/go/src/test1 |
official benchmark current impl $ go test -test.bench DeltaFIFOListKeys -test.run DeltaFIFOListKeys -count=10
goos: darwin
goarch: amd64
pkg: k8s.io/client-go/tools/cache
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkDeltaFIFOListKeys-12 77949 14921 ns/op
BenchmarkDeltaFIFOListKeys-12 82304 15016 ns/op
BenchmarkDeltaFIFOListKeys-12 82227 14624 ns/op
BenchmarkDeltaFIFOListKeys-12 80166 14884 ns/op
BenchmarkDeltaFIFOListKeys-12 83228 14847 ns/op
BenchmarkDeltaFIFOListKeys-12 80178 15167 ns/op
BenchmarkDeltaFIFOListKeys-12 80023 14952 ns/op
BenchmarkDeltaFIFOListKeys-12 78912 15647 ns/op
BenchmarkDeltaFIFOListKeys-12 73502 16353 ns/op
BenchmarkDeltaFIFOListKeys-12 73690 16669 ns/op
PASS
ok k8s.io/client-go/tools/cache 14.151s pull request impl $ go test -test.bench DeltaFIFOListKeys -test.run DeltaFIFOListKeys -count=10
goos: darwin
goarch: amd64
pkg: k8s.io/client-go/tools/cache
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkDeltaFIFOListKeys-12 161652 7621 ns/op
BenchmarkDeltaFIFOListKeys-12 156442 7468 ns/op
BenchmarkDeltaFIFOListKeys-12 158258 7577 ns/op
BenchmarkDeltaFIFOListKeys-12 159350 7639 ns/op
BenchmarkDeltaFIFOListKeys-12 156450 8074 ns/op
BenchmarkDeltaFIFOListKeys-12 160190 7664 ns/op
BenchmarkDeltaFIFOListKeys-12 159818 7426 ns/op
BenchmarkDeltaFIFOListKeys-12 156006 7522 ns/op
BenchmarkDeltaFIFOListKeys-12 169120 7368 ns/op
BenchmarkDeltaFIFOListKeys-12 151201 7358 ns/op
PASS
ok k8s.io/client-go/tools/cache 13.005s |
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. |
71462ab
to
96344ea
Compare
/assign @deads2k |
/kind cleanup |
@@ -235,8 +235,8 @@ func (f *FIFO) List() []interface{} { | |||
func (f *FIFO) ListKeys() []string { | |||
f.lock.RLock() | |||
defer f.lock.RUnlock() | |||
list := make([]string, 0, len(f.items)) | |||
for key := range f.items { | |||
list := make([]string, 0, len(f.queue)) |
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 isn't correct change - queue contains only "not yet consumed' items.
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.
However - this isn't correct change here.
In particular in Delete method - you may return something from items without touching queue:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/tools/cache/fifo.go#L218
So let's revert the change to fifo - and I can then approve the changes to delta_fifo.
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, done. The changes to fifo had rolled back.
Besides, would it be better to keep the internal keys of queue
and items
consistent in fifo?
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.
Good question - we would need to examine better the consequences and current usage.
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 isn't a correct change. /hold |
@NoicFank - please squash commits /hold cancel |
In function ListKeys, it better to use ‘queue’ instead of 'items' to get all the keys.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: NoicFank, wojtek-t 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 |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
In function ListKeys, it better to use ‘queue’ instead of 'items' to get all the keys.
Here is the reasons:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
benchmark results with DelfaFIFO:
current impl
pull request impl
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: