-
Notifications
You must be signed in to change notification settings - Fork 29
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
Add benchmarking CI that generates graphs on updates to master #530
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we envision this being updated and getting more complicated, then it would be worth considering a Python or OCaml script, but this is quite useful to have sooner than later so I'm willing to merge now and get the highlighted changes later.
Also consider using https://www.shellcheck.net/
tests/run-ci-benchmarks.sh
Outdated
COUNT=0 | ||
for TEST in ${SUCC}; do | ||
let COUNT=${COUNT}+1 | ||
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LEN=${#SUCC[@]}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent a while attempting to get this to work without success. I don't think it's a proper array although I don't know bash that well.
I'm inclined to leave the script as is for now and switch to a proper benchmarking system later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine (the issue is that there needs to be an extra pair of parens on the rhs of the defining TESTS).
tests/run-ci-benchmarks.sh
Outdated
INDEX=0 | ||
echo $SUCC | ||
echo $COUNT | ||
for TEST in ${SUCC}; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can iterate through the keys: http://stackoverflow.com/questions/3112687/ddg#3113285 so INDEX is unnecessary.
# If we're last, don't print a trailing comma. | ||
if [[ ${INDEX} -eq ${COUNT}-1 ]]; then | ||
# Hack to output JSON. | ||
cat << EOF >> ${JSON_FILE} | ||
{ | ||
"name": "${TEST}", | ||
"unit": "Seconds", | ||
"value": ${TIME} | ||
} | ||
EOF | ||
else | ||
cat << EOF >> ${JSON_FILE} | ||
{ | ||
"name": "${TEST}", | ||
"unit": "Seconds", | ||
"value": ${TIME} | ||
}, | ||
EOF | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps printf and a variable for the last comma would reduce code duplication here.
Something is still broken with the failing tests. I'm working on debugging it. |
FYI it passed on our fork. |
I'm not able to do anything on this PR, so a new one will be needed. |
This PR adds a CI workflow that generates performance graphs on every update to master. It stores the data as json in the
gh-pages
branch and renders the graphs on a Github Page. Here's an example of what this looks like with dummy data.This currently runs
cn
on all the (successful) .c files in the test suite. Eventually, we probably want to make a proper benchmark suite using something like Core_bench.Before this is merged, someone with permission needs to create a branch called
gh-pages
with empty contents. You can do this by grabbing the branchgh-pages-init
from our fork and pushing togh-pages
in this repository.Implements part of rems-project/cn-tutorial/issues/54.