Stream: git-wasmtime

Topic: wasmtime / PR #4421 Adds a github action to support x64 p...

Wasmtime GitHub notifications bot (Jul 09 2022 at 03:19):

jlb6740 opened PR #4421 from add-sightglass-benchmarking-actions to main:

…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.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jul 09 2022 at 03:19):

jlb6740 updated PR #4421 from add-sightglass-benchmarking-actions to main.

Wasmtime GitHub notifications bot (Jul 09 2022 at 03:20):

jlb6740 updated PR #4421 from add-sightglass-benchmarking-actions to main.

Wasmtime GitHub notifications bot (Jul 09 2022 at 03:21):

jlb6740 edited PR #4421 from add-sightglass-benchmarking-actions to main:

…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.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jul 09 2022 at 03:21):

jlb6740 edited PR #4421 from add-sightglass-benchmarking-actions to main:

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.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Jul 09 2022 at 03:24):

jlb6740 requested alexcrichton for a review on PR #4421.

Wasmtime GitHub notifications bot (Jul 09 2022 at 03:24):

jlb6740 requested fitzgen for a review on PR #4421.

Wasmtime GitHub notifications bot (Jul 09 2022 at 03:24):

jlb6740 requested abrown for a review on PR #4421.

Wasmtime GitHub notifications bot (Jul 11 2022 at 17:03):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jul 11 2022 at 17:03):

fitzgen created PR review comment:

Should this default to refs/heads/main?

Wasmtime GitHub notifications bot (Jul 11 2022 at 17:03):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jul 11 2022 at 17:03):

fitzgen created PR review comment:

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.

Wasmtime GitHub notifications bot (Jul 11 2022 at 17:03):

fitzgen created PR review comment:

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?

Wasmtime GitHub notifications bot (Jul 11 2022 at 19:20):

jlb6740 created PR review comment:

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.

Wasmtime GitHub notifications bot (Jul 11 2022 at 19:20):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Jul 11 2022 at 19:22):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Jul 11 2022 at 19:22):

jlb6740 created PR review comment:

Not anymore, will remove.

Wasmtime GitHub notifications bot (Jul 11 2022 at 19:26):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Jul 11 2022 at 19:26):

jlb6740 created PR review comment:

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.

Wasmtime GitHub notifications bot (Sep 14 2022 at 08:54):

jlb6740 updated PR #4421 from add-sightglass-benchmarking-actions to main.

Wasmtime GitHub notifications bot (Sep 14 2022 at 09:05):

jlb6740 updated PR #4421 from add-sightglass-benchmarking-actions to main.

Wasmtime GitHub notifications bot (Sep 14 2022 at 09:34):

jlb6740 updated PR #4421 from add-sightglass-benchmarking-actions to main.

Wasmtime GitHub notifications bot (Sep 14 2022 at 17:07):

jlb6740 updated PR #4421 from add-sightglass-benchmarking-actions to main.

Wasmtime GitHub notifications bot (Sep 14 2022 at 17:11):

jlb6740 updated PR #4421 from add-sightglass-benchmarking-actions to main.

Wasmtime GitHub notifications bot (Sep 14 2022 at 17:13):

jlb6740 updated PR #4421 from add-sightglass-benchmarking-actions to main.

Wasmtime GitHub notifications bot (Sep 14 2022 at 18:32):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 14 2022 at 18:32):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 14 2022 at 18:32):

alexcrichton created PR review comment:

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

blah blah blah words

/bench_x64

?

Wasmtime GitHub notifications bot (Sep 14 2022 at 18:32):

alexcrichton created PR review comment:

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

Wasmtime GitHub notifications bot (Sep 14 2022 at 18:32):

alexcrichton created PR review comment:

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.

Wasmtime GitHub notifications bot (Sep 14 2022 at 18:32):

alexcrichton created PR review comment:

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

Wasmtime GitHub notifications bot (Sep 14 2022 at 18:32):

alexcrichton created PR review comment:

Should this be TOKEN instead of WASMTIME_PUBLISHING_TOKEN?

Wasmtime GitHub notifications bot (Sep 14 2022 at 18:32):

alexcrichton created PR review comment:

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?

Wasmtime GitHub notifications bot (Sep 15 2022 at 16:23):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2022 at 16:23):

abrown created PR review comment:

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

Wasmtime GitHub notifications bot (Sep 15 2022 at 16:52):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2022 at 16:52):

abrown created PR review comment:

          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.

Wasmtime GitHub notifications bot (Sep 15 2022 at 16:52):

abrown created PR review comment:

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)

Wasmtime GitHub notifications bot (Sep 15 2022 at 16:52):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2022 at 18:52):

jlb6740 created PR review comment:

@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?

Wasmtime GitHub notifications bot (Sep 15 2022 at 18:52):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2022 at 19:13):

jlb6740 created PR review comment:

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


Wasmtime GitHub notifications bot (Sep 15 2022 at 19:13):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2022 at 19:17):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2022 at 19:17):

jlb6740 created PR review comment:

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.

Wasmtime GitHub notifications bot (Sep 15 2022 at 19:18):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2022 at 19:18):

jlb6740 created PR review comment:

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

Wasmtime GitHub notifications bot (Sep 15 2022 at 19:50):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2022 at 19:50):

jlb6740 created PR review comment:

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.

Wasmtime GitHub notifications bot (Sep 15 2022 at 19:52):

jlb6740 created PR review comment:

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

Wasmtime GitHub notifications bot (Sep 15 2022 at 19:52):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2022 at 19:57):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2022 at 19:57):

jlb6740 created PR review comment:

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

Wasmtime GitHub notifications bot (Sep 15 2022 at 20:51):

abrown created PR review comment:

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.

Wasmtime GitHub notifications bot (Sep 15 2022 at 20:51):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2022 at 20:52):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2022 at 20:52):

abrown created PR review comment:

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

Wasmtime GitHub notifications bot (Sep 15 2022 at 21:39):

jlb6740 created PR review comment:

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.

Wasmtime GitHub notifications bot (Sep 15 2022 at 21:39):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Sep 15 2022 at 21:53):

jlb6740 updated PR #4421 from add-sightglass-benchmarking-actions to main.

Wasmtime GitHub notifications bot (Sep 15 2022 at 21:55):

jlb6740 updated PR #4421 from add-sightglass-benchmarking-actions to main.

Wasmtime GitHub notifications bot (Sep 15 2022 at 22:53):

jlb6740 updated PR #4421 from add-sightglass-benchmarking-actions to main.

Wasmtime GitHub notifications bot (Sep 15 2022 at 23:24):

jlb6740 merged PR #4421.

Wasmtime GitHub notifications bot (Sep 16 2022 at 14:21):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Sep 16 2022 at 14:21):

alexcrichton created PR review comment:

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.

Wasmtime GitHub notifications bot (Sep 16 2022 at 14:22):

alexcrichton created PR review comment:

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

Wasmtime GitHub notifications bot (Sep 16 2022 at 14:22):

alexcrichton submitted PR review.

Last updated: Nov 22 2024 at 16:03 UTC