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.

Sign up for GitHub

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

Jump to bottom

Manual dispatch perf compare #443

Merged
merged 8 commits into from
Oct 5, 2021
Merged

Manual dispatch perf compare #443

merged 8 commits into from
Oct 5, 2021

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Sep 13, 2021 •
edited
Loading

This PR makes some changes to our perf comparison workflow. The current perf comparison is triggered by commits in a PR with the 'PR-benchmarking' label. After the change, the perf comparison is manually triggered with a workflow_dispatch event with a pull request number as its input. We can either run a workflow from the Github API in Actions, or later send the event from our bot when detecting certain comments in a PR.

This change achieves two goals:

  1. Allows triggering the perf runs manually. This avoids running perf runs for trivial commits, or building up a long work queue if there are many pushes in a short time.
  2. Allows always using our access token to access our perf scripts (we used to use GITHUB_TOKEN for the workflows which may not have access to our internal repos).

This PR:

  • moves the performance comparison jobs to a separate yml file
  • changes its trigger from pull_request to workflow_dispatch
  • changes the action we use to post results back to the PR. Now we also append to one same comment rather than simply adding a comment to the PR.
  • disables the micro benchmark workflow, as it seems not helpful.

Note that as workflow_dispatch cannot be triggered from a branch (only the workflow in master can be triggered), I am not able to test this. I tried to test the change with a pull_request trigger, and it worked fine, and I reverted the trigger (see: ea4980d). However, it is possible that this PR does not work, and requires further fix.

@mmtkgc-bot
Copy link

mmtkgc-bot commented Sep 13, 2021 •
edited
Loading

Running benchmarks for JikesRVM...
JikesRVM benchmarks finished.

@mmtk mmtk deleted a comment from qinsoon Sep 13, 2021
@mmtkgc-bot
Copy link

mmtkgc-bot commented Sep 13, 2021 •
edited
Loading

Running benchmarks for OpenJDK...
OpenJDK benchmarks finished.

@qinsoon qinsoon requested a review from caizixian September 13, 2021 05:43
@qinsoon qinsoon marked this pull request as ready for review September 13, 2021 05:43
@caizixian caizixian self-requested a review September 16, 2021 05:55
caizixian
caizixian approved these changes Sep 16, 2021
View reviewed changes
Copy link
Member

@caizixian caizixian left a comment

Choose a reason for hiding this comment

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

LGTM. Please take a look at that one line of comment I added. I think it's worth clarifying why we need that conditional.

@qinsoon
Copy link
Member Author

qinsoon commented Sep 16, 2021

LGTM. Please take a look at that one line of comment I added. I think it's worth clarifying why we need that conditional.

Yeah. That looks fine. Thanks for the review. I will merge this PR later (possibly right before #444 ).

@qinsoon qinsoon merged commit 7a6374b into master Oct 5, 2021
@qinsoon qinsoon deleted the manual-dispatch-perf-compare branch October 5, 2021 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caizixian caizixian caizixian approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@qinsoon @mmtkgc-bot @caizixian