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

Added GCP JMH Benchmark Workflow #770

Merged
merged 177 commits into from
Jul 17, 2023
Merged

Added GCP JMH Benchmark Workflow #770

merged 177 commits into from
Jul 17, 2023

Conversation

armughan11
Copy link
Collaborator

@armughan11 armughan11 commented Jun 16, 2023

Adds a workflow that is triggered via a "/benchmark" comment on a pull request. Clones the main and PR branch on the GCP instance and runs the ./gradlew jmh on each one of them, returning the results as a comment on the pull request. Running on my test instance for review, will change the credentials accordingly once approved.

@armughan11 armughan11 requested a review from msridhar July 15, 2023 02:30
@armughan11
Copy link
Collaborator Author

armughan11 commented Jul 15, 2023

Updated and tested after final changes and added VM start and stop steps.

Copy link
Collaborator

@msridhar msridhar left a comment

Choose a reason for hiding this comment

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

Looking great! Just a couple of final comments / questions

.github/workflows/jmh-benchmark.yml Show resolved Hide resolved
@@ -0,0 +1,9 @@
#!/bin/bash

cd $BRANCH_NAME/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this cd $BRANCH_NAME/ command in this script and also run_pr_benchmarks.sh?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just kept this for the sake of consistency across the two files. I am running the PR benchmark on the $BRANCH_NAME/pr/NullAway folder (the whole $BRANCH_NAME directory was set up to avoid errors due to any residual files from other PRs), and I am navigating back to the root directory after that's done. I can omit the cd $BRANCH_NAME from main, but that would mean that I will have to go up two directories to the $BRANCH_NAME folder in the step where I run the gcloud ssh command. Happy to make those changes if they are more suitable @msridhar. Thank you!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, it's fine to keep, was just curious.

Copy link
Collaborator

@msridhar msridhar left a comment

Choose a reason for hiding this comment

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

I think this is good to go! Will merge once we do some more testing. Thanks again @armughan11!

@@ -0,0 +1,9 @@
#!/bin/bash

cd $BRANCH_NAME/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, it's fine to keep, was just curious.

@msridhar
Copy link
Collaborator

msridhar commented Jul 17, 2023

Update: I successfully tested this change on my fork!

https://github.com/msridhar/NullAway/actions/runs/5580019520/jobs/10196359492
msridhar#10 (comment)

This was with a PR that deliberately introduced a slowdown. I did a couple of tweaks in naming based on that.

@armughan11 I have two more comments / questions for you.

  1. Benchmarking can take a while, and there is no feedback on the PR that anything is happening until the benchmarks complete. I was wondering, as a first thing could the workflow add a thumbs up to the /benchmark comment, to show that it is running? It seems possible, e.g., using this action (see here).
  2. Regarding the step to stop the VM running with always(), just curious, will this step run even if the job is canceled?

@armughan11
Copy link
Collaborator Author

Thanks for the update @msridhar!

  1. This should be doable, I will look into it. Ideally, I would add this step right after the GCP authentication is complete or right at the beginning, but if you want the reaction after a specific step, kindly let me know, so I can make the changes accordingly.
  2. Yes! That's why you would find the cleanup step with always() as well, which ensures that we don't end up with cloning errors for subsequent runs due to residual files. In short, steps marked always() aren't cancellable.

@msridhar
Copy link
Collaborator

  1. This should be doable, I will look into it. Ideally, I would add this step right after the GCP authentication is complete or right at the beginning, but if you want the reaction after a specific step, kindly let me know, so I can make the changes accordingly.

I was just thinking right at the beginning, to indicate something is running.

  1. Yes! That's why you would find the cleanup step with always() as well, which ensures that we don't end up with cloning errors for subsequent runs due to residual files. In short, steps marked always() aren't cancellable.

👍

@armughan11
Copy link
Collaborator Author

Just pushed a change. This should add the reaction once the job starts! @msridhar

@msridhar
Copy link
Collaborator

Confirmed that adding the reaction works: msridhar#10 (comment)

I think this is good to go. I'm going to land it and test on the main NullAway repo 🙂

@msridhar msridhar enabled auto-merge (squash) July 17, 2023 23:35
@msridhar msridhar merged commit e7ec4b9 into uber:master Jul 17, 2023
8 checks passed
@msridhar
Copy link
Collaborator

Thanks again for the great contribution @armughan11!

msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 18, 2023
msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 18, 2023
msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023
msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023
msridhar added a commit to msridhar/NullAway that referenced this pull request Jul 19, 2023
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.

None yet

3 participants