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

jlb6740 · 2022-07-09T03:19:07Z

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 · 2022-07-09T03:24:09Z

This patch is a replacement for #3740

Results currently look like this: jlb6740#3 (comment)

fitzgen

Excited for this!

fitzgen · 2022-07-11T16:58:55Z

.github/workflows/performance.yml

+  issue_comment:
+    types: [created, edited]
+  pull_request_review_comment:
+    types: [created, edited]


Do these triggers cover a comment (not review) on a pull request? I imagine that could fall under either of these buckets, but just want to make sure that we do actually allow a comment (as opposed to a review) on a pull request to trigger this.

Yes, both. The top covers an issue comment (not review) and I've tested this. I haven't tried to do a pull_request_review_comment but this here https://docs.github.com/en/github-ae@latest/actions/using-workflows/events-that-trigger-workflows#pull_request_review_comment explains it should work in that instance.

fitzgen · 2022-07-11T16:59:45Z

.github/workflows/performance.yml

+        sg_commit:
+          description: 'Sightglass ref'
+          type: string
+          default: '81c425a'


Should this default to refs/heads/main?

Yeah, wasn't sure if we wanted to bump it every now and then or just use head. Currently the way backend is written I think this needs to be a commit SHA, but will look to change it to a symbolic ref in a future iteration.

fitzgen · 2022-07-11T17:01:05Z

.github/workflows/performance.yml

+
+# Env variables
+env:
+  SG_COMMIT: 649509c


Is this used anywhere? Also, why is it a different hash from sg_commit above? If it is actually used, maybe we could have a comment giving a little context here?

Not anymore, will remove.

alexcrichton · 2022-07-11T17:05:48Z

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 · 2022-07-12T21:40:18Z

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 · 2022-07-13T15:18:00Z

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.

…glass 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 · 2022-08-19T17:51:51Z

@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 · 2022-08-19T17:56:50Z

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

jlb6740 · 2022-09-14T15:55:38Z

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

alexcrichton

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 · 2022-09-14T18:23:11Z

.github/workflows/performance.yml

+  pull_request_review_comment:
+    types: [created, edited]


The Wasmtime_Repo_On_PR_Comment job below filters on github.event_name == 'pull_request_review' so I think this action is never used as part of the trigger here?

alexcrichton · 2022-09-14T18:23:44Z

.github/workflows/performance.yml

+    runs-on: ubuntu-latest
+    if: |
+      (github.event_name == 'pull_request_review') &&
+      (startsWith(github.event.review.body, '/bench_x64')) &&


Could this perhaps change to contains instead of startsWith to allow something like

blah blah blah words

/bench_x64

?

alexcrichton · 2022-09-14T18:25:23Z

.github/workflows/performance.yml

+      - name: Checkout main from bytecodealliance/wasmtime
+        uses: actions/checkout@v3
+        with:
+          ref: 'main'


Would it be possible to use the git merge-base with the main branch of wasmtime and the previously benchmarked commit? That way the perf results could be more stable over time for comparison with a PR perhaps.

If I understand what git merge-base is implying/doing we are in fact using the merged version of the patch to test. On the pull_request review side, the ref_name (for pull_request_review) is actually referring to the merged version of the patch:

git fetch wasmtime refs/pull//merge:refs/remotes/wasmtime/pull//merge
git checkout wasmtime/pull/${{ github.ref_name }} -b ${{ github.ref_name }}

You can see that here where I am testing this in my fork of wasmtime:
https://github.com/jlb6740/wasmtime/actions/runs/3062995084/jobs/4944585819

So, and I haven't tested this yet, but I believe if someone tried to trigger a perf run using a patch that github is saying does not merge cleanly into mainline then that request would fail.

So I think this is already working as expected but let me know and we can iterate. 👍

Ah ok this makes sense, I always wondered what GitHub was doing here relative to merge commits and whatnot since testing merges is not really communicated all that well in the UI. In any case if the baseline changes over time I think that's reasonable as well since it's still testing where the only changes are the PR in question.

alexcrichton · 2022-09-14T18:25:50Z

.github/workflows/performance.yml

+          repository: ${{ env.repository }}
+          ref: ${{ env.event.ref_name }}


Are these two variables necessary to specify? I would assume that they could be omitted since the push itself is triggering the workflow

alexcrichton · 2022-09-14T18:27:08Z

.github/workflows/performance.yml

+      - name: Publish Results
+        run: |
+          curl -X POST -H "Accept: application/vnd.github.v3+json" \
+          -H "Authorization: token ${{ secrets.WASMTIME_PUBLISHING_TOKEN }}" \


Should this be TOKEN instead of WASMTIME_PUBLISHING_TOKEN?

Good catch. This is why this is difficult to test. I am simulating all of this on my forked branch with it's own master and default branch yet things will work differently once merged.

Actually .. no, this was correct. I'd created a WASMTIME_PUBLISHING_TOKEN on the perf server. I think this is fine. Whether it works once deployed is TBD but I think we should be ok.

Ah right yes there's two sets of secrets, forgot!

alexcrichton · 2022-09-14T18:27:58Z

.github/workflows/performance.yml

+          git checkout wasmtime/pull/${{ github.ref_name }} -b ${{ github.ref_name }}
+          git submodule update --init --recursive
+          git checkout -b wasmtime/${{ github.ref }}/${{ github.sha }}
+          sudo apt install jq


I would naively expect this to already be installed on github actions workers, but did you find this to be necessary?

jq v1.6 is available on ubuntu-latest (see here)

Yep .. even says so in the logs that it is already at its latest values.

abrown

Looks fine to me; I think we can iterate on this some more once it is merged!

abrown · 2022-09-15T16:44:25Z

.github/workflows/performance.yml

+        || ('fitzgen' == github.event.review.user.login)
+        || ('jlb6740' == github.event.review.user.login)
+        || ('sparker-arm' == github.event.review.user.login)
+        || ('uweigand' == github.event.review.user.login))


No need to change this now but I did find the documentation for contains and it looks like this could work:

contains(fromJson('["abrown", "afonso360", "etc"]'), github.event.review.user.login)

@abrown .. This is definitely cleaner but I think it will match if someone created an account with the user name something like "abrown_badactor". Is that right? If so, looking here I don't see a way to do an exact match using the notion suggested. Any thoughts on that?

I don't think so; take a look at that contains link--my take is it matches if the item is in the array. But this is definitely not a high-priority thing.

abrown · 2022-09-15T16:49:03Z

.github/workflows/performance.yml

+          export commit_url=${{ github.event.pull_request._links.commits.href }}
+          echo $commit_url
+          echo $(curl -sSL $commit_url | jq -r '.[].commit.committer.name' | tail -n 1)
+          echo $(curl -sSL $commit_url | jq -r '.[].commit.committer.email' | tail -n 1)
+          git config user.name $(curl -sSL $commit_url | jq -r '.[].commit.committer.name' | tail -n 1)
+          git config user.email $(curl -sSL $commit_url | jq -r '.[].commit.committer.email' | tail -n 1)


Suggested change

export commit_url=${{ github.event.pull_request._links.commits.href }}

echo $commit_url

echo $(curl -sSL $commit_url | jq -r '.[].commit.committer.name' | tail -n 1)

echo $(curl -sSL $commit_url | jq -r '.[].commit.committer.email' | tail -n 1)

git config user.name $(curl -sSL $commit_url | jq -r '.[].commit.committer.name' | tail -n 1)

git config user.email $(curl -sSL $commit_url | jq -r '.[].commit.committer.email' | tail -n 1)

export commit_url=${{ github.event.pull_request._links.commits.href }}

export committer_name=$(curl -sSL $commit_url | jq -r '.[].commit.committer.name' | tail -n 1)

export committer_email=$(curl -sSL $commit_url | jq -r '.[].commit.committer.email' | tail -n 1)

echo "Pushing ${commit_url} using account: ${committer_name} <${committer_email}>"

git config user.name ${committer_name}

git config user.email ${committer_email}

It seems like this is what we're trying to do here? This would eliminate the duplication.

The echos were simply to provide some verbosity to the logs. I'd left them there on purpose but can remove.

Yeah, I echoed a line to the log as well; I was just deduplicating the curl | jq code).

… message

jlb6740 · 2022-09-15T22:00:30Z

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 · 2022-09-15T23:24:15Z

Example output is here: jlb6740#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".

jlb6740 force-pushed the add-sightglass-benchmarking-actions branch 2 times, most recently from dceab09 to 11d757a Compare July 9, 2022 03:20

jlb6740 changed the title ~~Adds a github action to support x64 performance testing using a sight…~~ Adds a github action to support x64 performance testing using a sightglass Jul 9, 2022

jlb6740 requested review from abrown, alexcrichton and fitzgen July 9, 2022 03:24

fitzgen mentioned this pull request Jul 11, 2022

Add workflow to test perf using sightglass #3740

Closed

fitzgen approved these changes Jul 11, 2022

View reviewed changes

jlb6740 force-pushed the add-sightglass-benchmarking-actions branch 2 times, most recently from 7bda026 to a66b14f Compare September 14, 2022 09:34

Refactor patch to consolidate all steps to here.

a66b14f

jlb6740 force-pushed the add-sightglass-benchmarking-actions branch 2 times, most recently from 8cee1c7 to 9326da1 Compare September 14, 2022 17:13

Remove unused code

9326da1

alexcrichton approved these changes Sep 14, 2022

View reviewed changes

abrown approved these changes Sep 15, 2022

View reviewed changes

jlb6740 added 6 commits September 15, 2022 14:53

Remvoes unused pull_request_review_comment trigger

eaf9358

Match trigger word when contained anywhere in the pull request review…

705f66c

… message

Remove redundant repo and ref variables for wasmtime_commit

0678a0e

Minor comment update

551786e

Remove command to install jq

8f760fc

Remove printing of git config variables being used

48f56fd

jlb6740 added 3 commits September 15, 2022 14:53

Fix token for posting results

d1f2a38

Update message explaining pct_change for benchmark results

2ae992d

Revert TOKEN for publsh change

f6d7db9

Update message explaining results

56673bc

jlb6740 force-pushed the add-sightglass-benchmarking-actions branch from 3c23b0b to 56673bc Compare September 15, 2022 22:53

jlb6740 merged commit 27435ae into bytecodealliance:main Sep 15, 2022

		repository: ${{ env.repository }}
		ref: ${{ env.event.ref_name }}

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

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

Conversation

jlb6740 commented Jul 9, 2022 • edited Loading

jlb6740 commented Jul 9, 2022

fitzgen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jul 11, 2022

jlb6740 commented Jul 12, 2022

alexcrichton commented Jul 13, 2022

jlb6740 commented Aug 19, 2022

fitzgen commented Aug 19, 2022

jlb6740 commented Sep 14, 2022

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abrown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlb6740 commented Sep 15, 2022

jlb6740 commented Sep 15, 2022

jlb6740 commented Jul 9, 2022 •

edited

Loading