Skip to content

Add ion-java performance regression detector to GitHub workflow#2

Merged
linlin-s merged 3 commits intomasterfrom
linls-workflow
Jul 22, 2021
Merged

Add ion-java performance regression detector to GitHub workflow#2
linlin-s merged 3 commits intomasterfrom
linls-workflow

Conversation

@linlin-s
Copy link
Owner

Description of changes:

Workflow review guide:
This PR add the ability to build both ion-java and ion-java-benchmark-cli from the new commit and the previous commit when the new commit has been pushed. This is the first step of detecting ion-java performance regression. This workflow will be triggered when the action 'push commit to master branch' starts.
Step 1:
Checkout ion-java repository from the current commit(The one before push action happened) the build ion-java on GitHub host runner.
Step 2:
Checkout ion-java-benchmark-cli repository then build ion-java-benchmark-cli under the same host as ion-java.
Step 3:
Check the version of ion-java and ion-java-benchmark-cli by run: java -jar target/ion-java-benchmark-cli-0.0.1-SNAPSHOT-jar-with-dependencies.jar --version
This step will help to check whether the requested version of ion-java has been built.
Step 4:
Delete /.m2/repository which contains all dependencies from the last round build.
Step 5:
Checkout the ion-java-repository under the new commit(The commit pushed by developers) and build ion-java on the same host runner as the last build process.
Step 6:
Build ion-java-benchmark-cli again then check the version of ion-java to make sure the most recent ion-java has been under the benchmark process.

@linlin-s linlin-s changed the title Add ion-java performence regression detector to GitHub workflow Add ion-java performance regression detector to GitHub workflow Jul 14, 2021
Copy link
Collaborator

@cheqianh cheqianh left a comment

Choose a reason for hiding this comment

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

Looks good!


steps:
- uses: actions/checkout@v2

Copy link
Collaborator

Choose a reason for hiding this comment

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

Redundant empty lines.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Redundant empty lines.

Changes will be updated.

- name: Verify the CLI executes
run: java -jar target/ion-java-benchmark-cli-0.0.1-SNAPSHOT-jar-with-dependencies.jar --version
- name: clean maven repository
run : rm -r /home/runner/.m2
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to avoid deleting the whole directory?

E.g., The only different thing we have is different commits of ion-java, Is there anyway to replace ion-java only? This would make the workflow more efficient.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Is it possible to avoid deleting the whole directory?

E.g., The only different thing we have is different commits of ion-java, Is there anyway to replace ion-java only? This would make the workflow more efficient.

Is that mean replace the /.m2/repository/ion/ion-java? When the first version of ion-java has been built, all dependencies relevant to this version of ion-java will be downloaded to /.m2/repository/ion/ion-java, the reason I deleted the whole directory is to make sure when the new commit come in, and build ion-java-benchmark-cli will not reuse the dependency from the last step. But yes, if only delete the .m2/repository/ion/ion-jave would be more efficient, that would be better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Discussed offline.

name: Ion Jave performance regression detector

on:
push:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to add on: pull request too? So we can see the performance changes during code review section as well.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Do we need to add on: pull request too? So we can see the performance changes during code review section as well.

That's a good idea, changes will be updated in the following commit

…benchmark result from two commits of ion-java
Copy link
Collaborator

@cheqianh cheqianh left a comment

Choose a reason for hiding this comment

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

Looks good to me! Feel free to solve the previous comments.

- name: Build with Maven
run: mvn clean install
- name: Verify the CLI executes
- name: Check the version of ion-java and ion-java-benchmark-cli
Copy link
Collaborator

@cheqianh cheqianh Jul 22, 2021

Choose a reason for hiding this comment

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

A minor comment: We are not checking versions of both ion-java and ion-java-benchmark-cli right? We check version of ion-java-benchmark-cli and it prints the ion-java version it is using.

Copy link
Owner Author

Choose a reason for hiding this comment

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

A minor comment: We are not check versions of both ion-java and ion-java-benchmark-cli right? We check version of ion-java-benchmark-cli and it prints the ion-java version it is using.

We only need the version of ion-java, and after executing 'ion-java-benchmark-cli --version', both ion-java version and ion-java-benchmark-cli version will be printed out. Probably changing the name 'Check the version of ion-java and ion-java-benchmark-cli' to 'Check the version of ion-java' would be more reasonable.


- name: Test read preformance of the ion-java from the current commit
run: java -jar target/ion-java-benchmark-cli-0.0.1-SNAPSHOT-jar-with-dependencies.jar read testWorkflow.ion -o readPerformanceCurrent.ion -r ion

Copy link
Collaborator

Choose a reason for hiding this comment

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

Redundant empty lines.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Redundant empty lines.

Changes will be updated.

Copy link
Owner Author

@linlin-s linlin-s left a comment

Choose a reason for hiding this comment

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

The following commit will fix the minor problems mentioned from the last commit.

@linlin-s linlin-s merged commit 715ceba into master Jul 22, 2021
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.

2 participants