Adds a github action to support x64 performance testing using a sightglass

[jlb6740]

This github action allows performance testing using sightglass. The action is triggered either via a workflow dispatch or with the comment '/bench_x64', in a pull request. Once triggered the action will send a request to a private repository that supports using a self-hosted runner to do comparisons of "refs/feature/commit" vs "refs/heads/main" for wasmtime. If the action is triggered via a comment in a pull request (with '/bench_x64') then the commit referenced by the pull request is used for the comparison against refs/head/main. If triggered via a workflow dispatch the interface will request the commit to compare against refs/head/main. The results of the performance tests, run via sightglass, will be a table showing a percentage change in clock ticks in various stages requried for executing the benchmark, namely instantiate, compiliation, and execution. This patch is intended to be just a starting patch with much to tweak and improve. One of the TODOs will be adding support for aarch64 .. currently this patch supports only x64. Note also that the logic for actually doing the comparison and parsing the results occurs with the action associated with the private repo and so this patch itself (though the trigger) is fairly straight forward.

[jlb6740]

This patch is a replacement for #3740

Results currently look like this: https://github.com/jlb6740/wasmtime/pull/3#issuecomment-1179394254

[alexcrichton]

This is looking quite nice! Would it be possible to add bytecodealliance/wasmtime-sightglass-benchmarking/.github/workflows/main.yml to this repository as well? I think it'd be ideal if we could have everything in this repository if we can.

Instead of a curl to send a repository dispatch to the private repository I'd imagine that we could instead push to a branch? For example the commit that wants to be benchmarked could be checked out as part of this workflow and then pushed to a special branch name in the private repository. The "actually run the benchmark" perf run would then trigger on this special branch name and do everything it's already doing today.

Review-wise I think it'd be best to be able to edit the workflow publicly in this repository rather than juggling hidden state elsewhere.

[jlb6740]

This is looking quite nice! Would it be possible to add bytecodealliance/wasmtime-sightglass-benchmarking/.github/workflows/main.yml to this repository as well? I think it'd be ideal if we could have everything in this repository if we can.

I would definitely like that as well, but how is it possible?

Instead of a curl to send a repository dispatch to the private repository I'd imagine that we could instead push to a branch? For example the commit that wants to be benchmarked could be checked out as part of this workflow and then pushed to a special branch name in the private repository. The "actually run the benchmark" perf run would then trigger on this special branch name and do everything it's already doing today.

@alexcrichton Would you like to see this refactor before merging this patch? Currently I am not sure how what you describe would work. The yml file that executes commands to download the correct sightglass, mainline, and patch, I would think needs to be associated with the private repo. That self-hosted runner executes the run steps in the github action yml so that needs to be part of the private repo's github actions. By putting logic here to push a patch to the private server I don't see yet what that really accomplishes. It introduces a patch on the private server but for what purpose? Currently the private server (that gets it's run steps from the yml) downloads the patch. The patch is never duplicated on any other branch, it just lives at its home of its original feature branch. Also, I am not sure how that would work when multiple requests at the same time? In short, I don't yet see a way to accomplish the interesting steps in this repository alone. My thoughts are that while I completely agree from a review standpoint that it would be best to have all logic in one place, the private repository is that place and can be checked and has it's own review process. I don't think we have to sacrifice correctness. Just treat it like any other API .. when there is an issue just have to know which side to make the changes. From the wasmtime perspective, the curl statement sends a request to the private repository where the private repository is now a black box. If we want to scrutinize or update the logic the black box is doing, we just have to do it there.

Review-wise I think it'd be best to be able to edit the workflow publicly in this repository rather than juggling hidden state elsewhere.

Again, I do agree.

[alexcrichton]

I would envision adding the workflow to this repository as:

• The on: condition is only for branches that are named perf-test-*. This means it won't run for the main bytecodealliance repo
• The workflow added to this repo would run on a custom runner. The custom runner is only added to the private repo. This is ok since it won't actually run in the public repo.
• Otherwise the workflow in the private repo is basically the same, it checks out the branch whenever a push happens to perf-test-* and it could post results back to the PR when done.

Otherwise from this action added here the main difference would be instead of a curl a git push is done to a perf-test-* branch in the private repository.

Would you like to see this refactor before merging this patch?

Personally I would say yes. Having everything in one repo seems like a foundational shift relative to what you've currently got working which probably won't happen if we don't do it to start out with.

Also, I am not sure how that would work when multiple requests at the same time?

One idea would be for PR number N to push to perf-test-N so that way multiple PRs don't conflict.

the private repository is that place and can be checked and has it's own review process

The major downside, infrastructurally, is that you can't change atomically change the perf process in a PR to wasmtime. That means that the Wasmtime configuration to schedule a perf run must be kept in sync with the private repo's configuration. By having all the configuration in one place we can change everything at once in a single PR. Needing two PRs is not only cumbersome but runs the risk of breakage since the repos could get out of sync with each other.

[jlb6740]

@fitzgen @cfallin Hi guys .. had a good chat with @alexcrichton today and flushed out some concerns about the refactoring suggestion made. Its good to try the refactoring now. Since this is an anticipated patch, if there are any unanticipated roadblocks with the refactor we will look to merge this as is and address the refactor concern later, but we think the refactoring should work as expected. The idea is to have as much logic here as possible. Will start on that next week and hopefully have this ready for review the week after when @alexcrichton is back. I do anticipate that once the actions is updated there will be new comments and concerns to address before merger which is fine, so do anticipate that.

[fitzgen]

SGTM, really excited to get this up and running soon!

[jlb6740]

@alexcrichton Note, this latest patch is intended to address previous comments.

[jlb6740]

Looks great to me, thanks again for your patience!

I've got some minor nits below but it's fine to defer any or all of them to later if you'd prefer. Do you need me to configure any tokens or such in the repo before/after merging to have this work?

@alexcrichton No thanks for your patience! I think the refactoring works as imagined and is definitely an improved workflow as it combines everything in one patch and reduces logic. @alexcrichton @fitzgen @abrown I think this is ready to merge and iterate on. The only question I have is are the tokens workable. I believe they are, but of course those can be updated too.

[jlb6740]

Example output is here: https://github.com/jlb6740/wasmtime/pull/10 Note, the comment trigger /bench_x64 needs to be posted via the "review" comment box. It is not triggered by simply leaving a comment here in the "issue".