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

Continuous benchmarking #51

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Continuous benchmarking #51

wants to merge 28 commits into from

Conversation

LDeng0205
Copy link

This PR adds the continuous benchmarking. It currently runs with Github hosted runners; we are in the process of setting up russell as self-hosted runner to run the benchmarks. This PR comes with another PR in the benchmarks repo that makes some modifications to the runner script.

@LDeng0205 LDeng0205 requested review from cmnrd and lhstrh March 11, 2022 08:34
@petervdonovan petervdonovan force-pushed the continuous-benchmarking branch 3 times, most recently from 8f4cb40 to 4d1150a Compare July 10, 2022 02:21
@petervdonovan petervdonovan force-pushed the continuous-benchmarking branch 2 times, most recently from 5dc3ec6 to 8ed049e Compare July 12, 2022 21:02
@petervdonovan
Copy link
Contributor

There might be a little bit of meaningless noise in this PR while I try to setup GitHub Actions. There is an "unsubscribe" button in Github (on the right) for anyone who does not want useless messages.

@petervdonovan petervdonovan force-pushed the continuous-benchmarking branch 2 times, most recently from a1e7191 to b661909 Compare July 12, 2022 21:23
@petervdonovan petervdonovan marked this pull request as draft July 12, 2022 21:24
@petervdonovan petervdonovan force-pushed the continuous-benchmarking branch 6 times, most recently from 06fc272 to 26751d4 Compare July 13, 2022 07:03
@lf-lang lf-lang deleted a comment from github-actions bot Jul 13, 2022
@petervdonovan
Copy link
Contributor

petervdonovan commented Jul 15, 2022

The only thing holding this PR back right now is that the performance alerts are not appearing anymore, and I cannot figure out why not.

Edit: Resolved!

@petervdonovan petervdonovan marked this pull request as ready for review July 15, 2022 23:42
@lhstrh
Copy link
Member

lhstrh commented Sep 13, 2022

It seems we're incredibly close with this one. Is this ready to merge yet?

@petervdonovan
Copy link
Contributor

I think it might actually be fine to merge this. The other PR has to get moved, this seems like it can just get merged here. I still need to set up the self-hosted runner, but it seems imprudent to wait for the backlog of tasks to clear.

@petervdonovan
Copy link
Contributor

I keep being reminded of this PR, which is still potentially useful and which is still on the brink of being completed. After letting it sit for a while, I am beginning to get the feeling that hooking benchmarks into CI just might not be the way to go. CI has been a constant time-drain, and it will additionally become a security hazard if/when we link it up with our own servers. We may eventually decide that the most time-saving option in the long run is to focus on making sophisticated tests and benchmarks easy to run manually, on a server that we can just manually ssh into when necessary.

@lhstrh
Copy link
Member

lhstrh commented Nov 30, 2023

If we run benchmarks for feature branches on request and regularly (during times that CI is less busy) on main, I think the impact on wait times and security will be minimal. So I guess that makes it less "continuous" but that's OK because not all changes are expected to impact performance.

jobs:
benchmark:
name: Run C Benchmarks
runs-on: ubuntu-latest #FIXME: change to self-hosted after russel is set up.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
runs-on: ubuntu-latest #FIXME: change to self-hosted after russel is set up.
runs-on: Linux

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants