Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add DeepEquals generated code #11435

Merged
merged 3 commits into from
May 14, 2020
Merged

Add DeepEquals generated code #11435

merged 3 commits into from
May 14, 2020

Conversation

aanm
Copy link
Member

@aanm aanm commented May 8, 2020 •
edited by glibsm
Loading

Not relying on reflect.DeepEquals improves the DeepEqual performance
logic a lot. As an example, a benchmark was written for SpecEquals and
this commit was able to reduce the number of allocations and bytes per
operation to 0.

BenchmarkSpecEquals/Reflected_SpecEquals-8     98300  12325 ns/op  3117 B/op  68 allocs/op
BenchmarkSpecEquals/Generated_SpecEquals-8   3570753    354 ns/op     0 B/op   0 allocs/op

Note for reviewers: I had this sitting on my side for a while, I didn't submitted because of k8s structures but since we now control the k8s structures we can use these generated methods in our advantage.

@aanm aanm added the sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. label May 8, 2020
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@maintainer-s-little-helper
Copy link

Commit ccb77b6a40bf494b049a515bbb44b530d223154c does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. labels May 8, 2020
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@aanm aanm force-pushed the pr/add-small-scalability-imprevements branch from 61b629a to 2a0db53 Compare May 8, 2020 20:13
@maintainer-s-little-helper
Copy link

Commit ccb77b6a40bf494b049a515bbb44b530d223154c does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@aanm aanm added the release-note/major This PR introduces major new functionality to Cilium. label May 8, 2020
@aanm aanm force-pushed the pr/add-small-scalability-imprevements branch from 2a0db53 to 540476e Compare May 8, 2020 20:14
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 8, 2020
@aanm
Copy link
Member Author

aanm commented May 8, 2020

test-me-please

@aanm aanm force-pushed the pr/add-small-scalability-imprevements branch from 540476e to 1cab767 Compare May 11, 2020 09:07
@aanm
Copy link
Member Author

aanm commented May 11, 2020

test-me-please

@aanm aanm force-pushed the pr/add-small-scalability-imprevements branch from 1cab767 to af84424 Compare May 11, 2020 12:34
@aanm
Copy link
Member Author

aanm commented May 11, 2020

test-me-please

@aanm aanm force-pushed the pr/add-small-scalability-imprevements branch from af84424 to 8a5a586 Compare May 11, 2020 20:44
@aanm
Copy link
Member Author

aanm commented May 11, 2020

test-me-please

@aanm aanm changed the title "Small" scalability improvements Add DeepEquals generated code May 11, 2020
@pchaigno pchaigno requested review from jrajahalme and removed request for jrajahalme May 11, 2020 20:50
@aanm aanm force-pushed the pr/add-small-scalability-imprevements branch from 8a5a586 to 2df1a56 Compare May 11, 2020 21:01
@aanm
Copy link
Member Author

aanm commented May 11, 2020

test-me-please

@aanm aanm force-pushed the pr/add-small-scalability-imprevements branch 2 times, most recently from e2a3ada to 0b62d22 Compare May 12, 2020 08:13
@aanm
Copy link
Member Author

aanm commented May 12, 2020

test-me-please

@coveralls
Copy link

coveralls commented May 12, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-1.0%) to 37.078% when pulling 609bdcc on pr/add-small-scalability-imprevements into 96384f0 on master.

@aanm aanm marked this pull request as ready for review May 12, 2020 09:04
@aanm
Copy link
Member Author

aanm commented May 12, 2020

retest-runtime

@aanm aanm marked this pull request as ready for review May 12, 2020 19:59
@nebril
Copy link
Member

nebril commented May 13, 2020

retest-runtime

@aanm aanm force-pushed the pr/add-small-scalability-imprevements branch from 0b62d22 to 0cb36ab Compare May 13, 2020 11:56
@aanm
Copy link
Member Author

aanm commented May 13, 2020

test-me-please

@aanm aanm force-pushed the pr/add-small-scalability-imprevements branch from 0cb36ab to babc8ff Compare May 13, 2020 12:05
@aanm
Copy link
Member Author

aanm commented May 13, 2020

test-me-please

nebril
nebril reviewed May 13, 2020
View reviewed changes
Copy link
Member

@nebril nebril left a comment

Choose a reason for hiding this comment

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

Good stuff! One question inline, one GH issue to create.

Makefile Outdated Show resolved Hide resolved
pkg/cidr/cidr.go
@@ -33,6 +33,25 @@ type CIDR struct {
*net.IPNet
}

func (in *CIDR) DeepEqual(other *CIDR) bool {
if other == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we also check for in being nil here (Like in CiliumNetworkPolicy below)?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's up to the caller to check for the receiver to be nil.

pkg/policy/api/selector.go Outdated
case (in == nil) && (other == nil):
return true
}
// Once k8s upstream library supports DeepEqual we can remove this reflect
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to have a gh issue tracking that.

@aanm aanm force-pushed the pr/add-small-scalability-imprevements branch from babc8ff to 61e3eb7 Compare May 13, 2020 13:29
@aanm
Copy link
Member Author

aanm commented May 13, 2020

test-me-please

@aanm aanm force-pushed the pr/add-small-scalability-imprevements branch from 61e3eb7 to 7721d00 Compare May 13, 2020 14:06
@aanm
Copy link
Member Author

aanm commented May 13, 2020

test-me-please

@aanm aanm force-pushed the pr/add-small-scalability-imprevements branch 2 times, most recently from a4630ab to 17d6403 Compare May 13, 2020 14:32
@aanm
Copy link
Member Author

aanm commented May 13, 2020

test-me-please

@aanm aanm mentioned this pull request May 13, 2020
Merged
aanm added 3 commits May 13, 2020 17:39
@aanm
add deepequals library generator
30b3257 
Signed-off-by: André Martins <[email protected]>
@aanm
add golang markers for deep equal generator
7128bd1 
Not relying on reflect.DeepEquals improves the DeepEqual performance
logic a lot. As an example, a benchmark was written for SpecEquals and
this commit was able to reduce the number of allocations and bytes per
operation to 0.

```
BenchmarkSpecEquals/Reflected_SpecEquals-8     98300  12325 ns/op  3117 B/op  68 allocs/op
BenchmarkSpecEquals/Generated_SpecEquals-8   2628786    452 ns/op     0 B/op   0 allocs/op
```

Unfortunately the api/v1/models markers had to be put manually.

Signed-off-by: André Martins <[email protected]>
@aanm
use deepequal generated code
609bdcc 
This commit uses the deepqual code generated in the previous code.

Signed-off-by: André Martins <[email protected]>
@aanm aanm force-pushed the pr/add-small-scalability-imprevements branch from 17d6403 to 609bdcc Compare May 13, 2020 15:39
@aanm aanm requested a review from a team May 13, 2020 15:39
@aanm
Copy link
Member Author

aanm commented May 13, 2020

test-me-please

@aanm
Copy link
Member Author

aanm commented May 13, 2020

retest-net-next

christarazi
christarazi approved these changes May 14, 2020
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

Nice!

@aanm
Copy link
Member Author

aanm commented May 14, 2020 •
edited
Loading

hit #11181 #11512

@aanm aanm merged commit 2d34336 into master May 14, 2020
@aanm aanm deleted the pr/add-small-scalability-imprevements branch July 6, 2020 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vadorovsky vadorovsky vadorovsky approved these changes

@christarazi christarazi christarazi approved these changes

@nebril nebril nebril approved these changes

@tklauser tklauser tklauser approved these changes

Assignees
No one assigned
Labels
release-note/major This PR introduces major new functionality to Cilium. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@aanm @coveralls @nebril @tklauser @christarazi @vadorovsky