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 benchmark for updates and get with/without server-side apply #80944

Merged
merged 1 commit into from
Aug 17, 2019
Merged

Add benchmark for updates and get with/without server-side apply #80944

merged 1 commit into from
Aug 17, 2019

Conversation

apelisse
Copy link
Member

@apelisse apelisse commented Aug 3, 2019

Add a high-level benchmark for repeated updates and get when server-side apply is enabled or not, as a way to compare the impact of enabling the feature.

The output is hard to read though because I haven't managed to disable logs altogether.

Still, the results are:

benchmark                                         old ns/op     new ns/op     delta
BenchmarkNoServerSideApply/Get-12                 10460524      10261256      -1.90%
BenchmarkNoServerSideApply/RepeatedUpdates-12     11307965      11474860      +1.48%
BenchmarkServerSideApply/Get-12                   11043359      10495828      -4.96%
BenchmarkServerSideApply/RepeatedUpdates-12       20689213      16360253      -20.92%

benchmark                                         old allocs     new allocs     delta
BenchmarkNoServerSideApply/Get-12                 641            637            -0.62%
BenchmarkNoServerSideApply/RepeatedUpdates-12     2081           2081           +0.00%
BenchmarkServerSideApply/Get-12                   2373           879            -62.96%
BenchmarkServerSideApply/RepeatedUpdates-12       19920          8909           -55.28%

Get is not super relevant (especially with/without SSA because it's mostly dominated by overhead), we would have to insert many more objects to actually show the difference.

What type of PR is this?
/kind cleanup

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/L Denotes a PR that changes 100-499 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. labels Aug 3, 2019
@apelisse
Copy link
Member Author

apelisse commented Aug 3, 2019

cc @jennybuckley @wojtek-t @lavalamp

@k8s-ci-robot k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 3, 2019
@apelisse
Copy link
Member Author

apelisse commented Aug 3, 2019 •
edited
Loading

Speed-wise, for PATCH, it went from 1.82x to 1.44x, allocations went from 20x to 8x.

@apelisse
Copy link
Member Author

apelisse commented Aug 3, 2019

/retest

@k8s-ci-robot k8s-ci-robot added area/dependency Issues or PRs related to dependency changes and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 12, 2019
@apelisse
Copy link
Member Author

Updated the code, we now get the following results:

NoServerSideApply/Get-12               200  34912437 ns/op  3061219 B/op   28242 allocs/op
NoServerSideApply/RepeatedUpdates-12  1000  10976103 ns/op   169518 B/op    2081 allocs/op
NoServerSideApply/Post-12              300  21495704 ns/op  6149313 B/op   56480 allocs/op
ServerSideApply/Get-12                 100  52903182 ns/op  6515623 B/op   45408 allocs/op
ServerSideApply/RepeatedUpdates-12     500  16272308 ns/op   806456 B/op    8913 allocs/op
ServerSideApply/Post-12                300  23594350 ns/op 11460240 B/op  114095 allocs/op

@apelisse apelisse force-pushed the add-update-bench branch 2 times, most recently from 5625d70 to 00e1f06 Compare August 14, 2019 00:16
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 14, 2019
lavalamp
lavalamp reviewed Aug 15, 2019
View reviewed changes
test/integration/apiserver/apply/apply_test.go Outdated
return nil
}

func benchGetPod(client kubernetes.Interface, pod v1.Pod, num int) func(*testing.B) {
Copy link
Member

Choose a reason for hiding this comment

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

This is List, not GET

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct

lavalamp
lavalamp reviewed Aug 15, 2019
View reviewed changes
test/integration/apiserver/apply/apply_test.go
b.ReportAllocs()
for i := 0; i < b.N; i++ {
c := make(chan error)
for j := 0; j < parallel; j++ {
Copy link
Member

Choose a reason for hiding this comment

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

Why do things in parallel?

Copy link
Member Author

Choose a reason for hiding this comment

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

We were suspecting this was making things even worse (closer to real-life system)

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it's necessary to capture that in the benchmark, but I guess it doesn't hurt...

@lavalamp
Copy link
Member

/approve

Can you fix the tests?

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apelisse, lavalamp

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

@apelisse
Copy link
Member Author

I haven't changed tested code nor really test code, just new benchmark, so I suspect this must be flakes
/retest

@lavalamp
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2019
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 3f00331 into kubernetes:master Aug 17, 2019
@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lavalamp lavalamp lavalamp left review comments

@msau42 msau42 Awaiting requested review from msau42

@rramkumar1 rramkumar1 Awaiting requested review from rramkumar1

Assignees

@lavalamp lavalamp

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/dependency Issues or PRs related to dependency changes area/test 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.16
Development

Successfully merging this pull request may close these issues.
4 participants
@apelisse @lavalamp @k8s-ci-robot @fejta-bot