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

Fix benchmark CI pipeline #460

Merged
merged 3 commits into from
Jul 10, 2024
Merged

Fix benchmark CI pipeline #460

merged 3 commits into from
Jul 10, 2024

Conversation

henrybear327
Copy link
Collaborator

Recompile dhrystone.elf and coremark.elf in order to fix the benchmark CI pipeline, as discussed at #455 (comment)

References:

@henrybear327 henrybear327 marked this pull request as draft June 16, 2024 20:09
@henrybear327 henrybear327 self-assigned this Jun 16, 2024
jserv
jserv reviewed Jun 16, 2024
View reviewed changes
Copy link
Contributor

@jserv jserv left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Benchmarks

Benchmark suite Current: c9d8c6d Previous: 867a53d Ratio
Dhrystone 3.88 Average DMIPS over 10 runs 3.44 Average DMIPS over 10 runs 0.89
Coremark 0.004 Average iterations/sec over 10 runs 0.004 Average iterations/sec over 10 runs 1

This comment was automatically generated by workflow using github-action-benchmark.

@henrybear327
Copy link
Collaborator Author

@jserv do you still have the documentation/Makefile/etc. for compiling coremark.elf?

jserv
jserv requested changes Jun 17, 2024
View reviewed changes
Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Strip the prebuilt CoreMark and Dhrystone ELF files.

@jserv
Copy link
Contributor

jserv commented Jun 17, 2024

do you still have the documentation/Makefile/etc. for compiling coremark.elf?

Run git log build/coremark.elf to figure out the steps to rebuild CoreMark.

@henrybear327
Copy link
Collaborator Author

henrybear327 commented Jul 3, 2024 •
edited
Loading

do you still have the documentation/Makefile/etc. for compiling coremark.elf?

Run git log build/coremark.elf to figure out the steps to rebuild CoreMark.

Replaced coremark using the binary compiled from rv32emu-prebuilt

@henrybear327
Copy link
Collaborator Author

@jserv The performance is still slow after replacing both binaries (using the ones compiled from rv32emu-prebuilt)

@jserv
Copy link
Contributor

jserv commented Jul 3, 2024

@jserv The performance is still slow after replacing both binaries (using the ones compiled from rv32emu-prebuilt)

@vacantron, Can you check?

@henrybear327 henrybear327 force-pushed the ci/fix_benchmark_pipeline branch 18 times, most recently from 80f3ca2 to 3b7fdf5 Compare July 4, 2024 00:51
jserv
jserv requested changes Jul 9, 2024
View reviewed changes
Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

It is confusing to have benchmark.yml and benchmark-new.yml.

@henrybear327 henrybear327 force-pushed the ci/fix_benchmark_pipeline branch 4 times, most recently from 1da388c to 0bf82b8 Compare July 9, 2024 12:23
@henrybear327
Copy link
Collaborator Author

It is confusing to have benchmark.yml and benchmark-new.yml.

Synced offline with @jserv, this is a temporary testing method.

A new discovery is that if I move the benchmark pipeline to main.yml, the latest changes on the pipeline can be reflected directly with the PR.

I currently have no explanation for this, but to speed up development I will keep the benchmark pipeline in main.yml for now.

@henrybear327 henrybear327 marked this pull request as draft July 9, 2024 13:40
@jserv
Copy link
Contributor

jserv commented Jul 9, 2024

I currently have no explanation for this, but to speed up development I will keep the benchmark pipeline in main.yml for now.

It sounds acceptable. Let's get the benchark work again.

@henrybear327 henrybear327 force-pushed the ci/fix_benchmark_pipeline branch 2 times, most recently from ac20c15 to 48820ff Compare July 9, 2024 16:04
@henrybear327
Copy link
Collaborator Author

henrybear327 commented Jul 9, 2024 •
edited
Loading

Good news! The pipeline is finally back online with normal numbers again! Coremark is currently compiled using rv32im and rv32imafc (thanks to @vacantron for the suggestion). Both can be executed without failure!

@jserv do you happen to remember what were the parameters you used for Coremark? It's not found within the past commit messages but we would need to supply this in the benchmark script. Currently, the CI is using 0x0 0x0 0x66 30000 7 1 2000 (>= 15000 iterations should be more than 10s of execution time)

Thank you!

@henrybear327 henrybear327 force-pushed the ci/fix_benchmark_pipeline branch 2 times, most recently from 202fb65 to 5698354 Compare July 9, 2024 16:17
@henrybear327 henrybear327 requested a review from jserv July 9, 2024 16:19
@henrybear327 henrybear327 marked this pull request as ready for review July 9, 2024 16:19
@henrybear327
Copy link
Collaborator Author

Though I am not sure of the root cause of this error when executing Coremark

The Coremark with rv32imafc extension should work normally now. It seems there are some problems with the certain extension combination.

I have tried rv32im (https://github.com/sysprog21/rv32emu/commits/master/build/coremark.elf), also works!

@jserv
Copy link
Contributor

jserv commented Jul 9, 2024

do you happen to remember what were the parameters you used for Coremark? It's not found within the past commit messages but we would need to supply this in the benchmark script. Currently, the CI is using 0x0 0x0 0x66 30000 7 1 2000 (>= 15000 iterations should be more than 10s of execution time)

Prior to this pull request, there was no parameter passing to CoreMark.

@henrybear327
Copy link
Collaborator Author

do you happen to remember what were the parameters you used for Coremark? It's not found within the past commit messages but we would need to supply this in the benchmark script. Currently, the CI is using 0x0 0x0 0x66 30000 7 1 2000 (>= 15000 iterations should be more than 10s of execution time)

Prior to this pull request, there was no parameter passing to CoreMark.

I took the arguments from https://github.com/HidetaroTanaka/coremark/blob/main/Makefile#L103 and https://github.com/HidetaroTanaka/coremark/blob/main/Makefile#L107.

According to the experiment, executing coremark for 1 second can perform around 1200 iterations. Also, if the execution time is less than 10s, coremark will fail with a non-zero exit code. So I set the iteration to 20000 for now.

@henrybear327
Copy link
Collaborator Author

Known issues:

  • Benchmark / Performance regression check: it's already deleted, but still being executed
  • CI / benchmark: can't access secrets.RV32EMU_BENCH_TOKEN (I believe after merging the PR this will be resolved, as this is a security feature so we can't leak the secret using a PR from a contributor)

@jserv
Copy link
Contributor

jserv commented Jul 10, 2024

Move the commit "Fix typo and remove outdated comment" out of this pull request.

@henrybear327
Update dhrystone.elf and coremark.elf
cf69d83 
dhrystone is compiled with sysprog21/rv32emu-prebuilt#1

coremark is compiled with sysprog21/rv32emu-prebuilt@1dac9bb,
but setting -march=rv32im

Signed-off-by: Chun-Hung Tseng <henrybear327@gmail.com>
@henrybear327
Add coremark arguments
2ee375b 
Signed-off-by: Chun-Hung Tseng <henrybear327@gmail.com>
@henrybear327
Move benchmark run to main.yml file
0e13f01 
This commit also fixes pip3 install error: externally-managed-environment

Reference:
- https://stackoverflow.com/questions/75608323/how-do-i-solve-error-externally-managed-environment-every-time-i-use-pip-3

Signed-off-by: Chun-Hung Tseng <henrybear327@gmail.com>
@henrybear327 henrybear327 mentioned this pull request Jul 10, 2024
Merged
@henrybear327
Copy link
Collaborator Author

Move the commit "Fix typo and remove outdated comment" out of this pull request.

Done as requested (a new PR is created)

@henrybear327
Copy link
Collaborator Author

Move the commit "Fix typo and remove outdated comment" out of this pull request.

Done as requested (a new PR is created)

Since there are no code changes, the CI pipeline won't be triggered, thus, passing all of them!

@jserv jserv merged commit a2e3aaa into sysprog21:master Jul 10, 2024
9 checks passed
@henrybear327 henrybear327 deleted the ci/fix_benchmark_pipeline branch July 10, 2024 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jserv jserv Awaiting requested review from jserv

Assignees

@henrybear327 henrybear327

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@henrybear327 @jserv @vacantron