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.

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

Implement equality for Values #126

Merged
merged 2 commits into from Nov 7, 2019

Conversation

@apelisse
Copy link
Contributor

apelisse commented Nov 6, 2019

Equality doesn't require the same amount of sophistication than ordering
comparison. Implement an Equal function that does just that but does it
much faster.

Benchmark results show up to 96% improvement:

benchmark                                                         old ns/op     new ns/op     delta
BenchmarkDeducedSimple-12                                         110083        103256        -6.20%
BenchmarkDeducedNested-12                                         374366        374160        -0.06%
BenchmarkDeducedNestedAcrossVersion-12                            392864        405525        +3.22%
BenchmarkLeafConflictAcrossVersion-12                             89112         89070         -0.05%
BenchmarkMultipleApplierRecursiveRealConversion-12                1564330       1574620       +0.66%
BenchmarkOperations/Pod/Create-12                                 103693        103970        +0.27%
BenchmarkOperations/Pod/Apply-12                                  291760        291317        -0.15%
BenchmarkOperations/Pod/Update-12                                 193419        190470        -1.52%
BenchmarkOperations/Pod/UpdateVersion-12                          261692        251966        -3.72%
BenchmarkOperations/Node/Create-12                                152047        155710        +2.41%
BenchmarkOperations/Node/Apply-12                                 499187        473901        -5.07%
BenchmarkOperations/Node/Update-12                                299271        279142        -6.73%
BenchmarkOperations/Node/UpdateVersion-12                         438723        403125        -8.11%
BenchmarkOperations/Endpoints/Create-12                           12246         11940         -2.50%
BenchmarkOperations/Endpoints/Apply-12                            915806        924080        +0.90%
BenchmarkOperations/Endpoints/Update-12                           7155675       285092        -96.02%
BenchmarkOperations/Endpoints/UpdateVersion-12                    14278150      544040        -96.19%
BenchmarkOperations/CustomResourceDefinition/Create-12            1312734       1288472       -1.85%
BenchmarkOperations/CustomResourceDefinition/Apply-12             3346591       3376864       +0.90%
BenchmarkOperations/CustomResourceDefinition/Update-12            10681243      1758764       -83.53%
BenchmarkOperations/CustomResourceDefinition/UpdateVersion-12     19069925      2202330       -88.45%
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 6, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

value/less_test.go Outdated Show resolved Hide resolved
@apelisse apelisse force-pushed the apelisse:perf/improve-equality branch from 9cd3089 to 25f0e42 Nov 6, 2019
@k8s-ci-robot k8s-ci-robot added size/M and removed size/L labels Nov 6, 2019
@apelisse

This comment has been minimized.

Copy link
Contributor Author

apelisse commented Nov 6, 2019

Fixed!

apelisse added 2 commits Nov 6, 2019
Equality doesn't require the same amount of sophistication than ordering
comparison. Implement an Equal function that does just that but does it
much faster.

Benchmark results show up to 96% improvement:
```
benchmark                                                         old ns/op     new ns/op     delta
BenchmarkDeducedSimple-12                                         110083        103256        -6.20%
BenchmarkDeducedNested-12                                         374366        374160        -0.06%
BenchmarkDeducedNestedAcrossVersion-12                            392864        405525        +3.22%
BenchmarkLeafConflictAcrossVersion-12                             89112         89070         -0.05%
BenchmarkMultipleApplierRecursiveRealConversion-12                1564330       1574620       +0.66%
BenchmarkOperations/Pod/Create-12                                 103693        103970        +0.27%
BenchmarkOperations/Pod/Apply-12                                  291760        291317        -0.15%
BenchmarkOperations/Pod/Update-12                                 193419        190470        -1.52%
BenchmarkOperations/Pod/UpdateVersion-12                          261692        251966        -3.72%
BenchmarkOperations/Node/Create-12                                152047        155710        +2.41%
BenchmarkOperations/Node/Apply-12                                 499187        473901        -5.07%
BenchmarkOperations/Node/Update-12                                299271        279142        -6.73%
BenchmarkOperations/Node/UpdateVersion-12                         438723        403125        -8.11%
BenchmarkOperations/Endpoints/Create-12                           12246         11940         -2.50%
BenchmarkOperations/Endpoints/Apply-12                            915806        924080        +0.90%
BenchmarkOperations/Endpoints/Update-12                           7155675       285092        -96.02%
BenchmarkOperations/Endpoints/UpdateVersion-12                    14278150      544040        -96.19%
BenchmarkOperations/CustomResourceDefinition/Create-12            1312734       1288472       -1.85%
BenchmarkOperations/CustomResourceDefinition/Apply-12             3346591       3376864       +0.90%
BenchmarkOperations/CustomResourceDefinition/Update-12            10681243      1758764       -83.53%
BenchmarkOperations/CustomResourceDefinition/UpdateVersion-12     19069925      2202330       -88.45%
```
The current Less implementation has an exponential complexity. Replacing
Less with a Compare method avoids that problem.
@apelisse apelisse force-pushed the apelisse:perf/improve-equality branch from 25f0e42 to b45ef08 Nov 6, 2019
@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Nov 6, 2019
// LHS is less; return
return true
}
if fb.Value.Less(fa.Value) {

This comment has been minimized.

Copy link
@lavalamp

lavalamp Nov 7, 2019

Contributor

Can't believe I did this... whoops

@lavalamp

This comment has been minimized.

Copy link
Contributor

lavalamp commented Nov 7, 2019

/lgtm

Great

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 7, 2019
@k8s-ci-robot k8s-ci-robot merged commit 0ad1808 into kubernetes-sigs:master Nov 7, 2019
3 of 4 checks passed
3 of 4 checks passed
tide Not mergeable. Needs lgtm label.
Details
cla/linuxfoundation apelisse authorized
Details
pull-structured-merge-diff-gofmt Job succeeded.
Details
pull-structured-merge-diff-test Job succeeded.
Details
k8s-ci-robot added a commit that referenced this pull request Nov 11, 2019
Cherrypick #126 to 1.0 - Implement equality for Values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.