Skip to content
This repository has been archived by the owner on Apr 19, 2024. It is now read-only.

ci: run benchmarks on every pr and alert if perf is worse #200

Merged
merged 6 commits into from
Nov 28, 2023

Conversation

miparnisari
Copy link
Contributor

Close #199

Copy link
Contributor

@thrawn01 thrawn01 left a comment

Choose a reason for hiding this comment

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

Thank you for taking the time to make this PR! This is a great addition. I'm not sure yet why the bench test timed out at 30 minutes. But we should probably timeout the test after 10 minutes.

Makefile Outdated
@@ -18,6 +18,10 @@ test:
go tool cover -html coverage.out -o coverage.html; \
exit $$ret)

.PHONY: bench
bench:
go test ./... -bench . -benchtime 5s -timeout 0 -run=XXX -cpu 1 -benchmem
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of the tests bench concurrent access. I don't think we need -run=XXX -cpu 1

Copy link
Contributor Author

@miparnisari miparnisari Nov 9, 2023

Choose a reason for hiding this comment

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

👍 I'll remove -cpu 1 but keep -run=XXX since that is supposed to ensure that all benchmarks in all folders run

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I've used -run '^$' for this purpose to match an empty string which is (probably) impossible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know what i'm doing wrong, but i get an error

[21/11/23 4:36:35] ~/GitHub/gubernator (run-benchmarks) $ make bench
go test ./... -bench . -benchtime 5s -timeout 0 -run '^ -benchmem
/bin/sh: -c: line 0: unexpected EOF while looking for matching `''
/bin/sh: -c: line 1: syntax error: unexpected end of file
make: *** [bench] Error 2
[21/11/23 4:36:44] ~/GitHub/gubernator (run-benchmarks) $ 

@miparnisari
Copy link
Contributor Author

I'm not sure yet why the bench test timed out at 30 minutes.

@thrawn01 I think either BenchmarkLRUCache/Concurrent_writes or https://github.com/miparnisari/gubernator/blob/61e6cd7bd0e2d876431b8d085c5d0226ea16a894/lrucache_test.go#L551 timed out. Probably because it executed all the reads serially? 🤷‍♀️

But we should probably timeout the test after 10 minutes.

Each benchmark is configured to run for 5 seconds and the benchmark step in the github action are set to either 15 or 30 minutes

@miparnisari miparnisari marked this pull request as ready for review November 16, 2023 21:43
.github/workflows/on-pull-request.yml Outdated Show resolved Hide resolved
@Baliedge
Copy link
Contributor

@miparnisari Could we test the go-bench check to show it fails when exceeding the threshold? Maybe make a temporary change to add sleeps in the benchmarks?

@miparnisari
Copy link
Contributor Author

@Baliedge i would need to merge this first into master so that I have something to compare with :) But i can do that in a different PR.

Copy link
Contributor

@thrawn01 thrawn01 left a comment

Choose a reason for hiding this comment

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

Looks good to me, Does this require a GitHub API key in secrets?

@thrawn01
Copy link
Contributor

Never mind, I just answered my own question 😜

@thrawn01 thrawn01 merged commit 5a43d52 into mailgun:master Nov 28, 2023
3 checks passed
@miparnisari miparnisari mentioned this pull request Nov 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run benchmarks!
3 participants