Skip to content

Add benchmark suite to CI #15696

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

Merged
merged 5 commits into from
Sep 25, 2024
Merged

Add benchmark suite to CI #15696

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4e0559a
Add version benchmark to CI
kocsismate Sep 1, 2024
efd87aa
Review fixes
kocsismate Sep 7, 2024
23fcb24
Measure results for a baseline PHP version
kocsismate Sep 12, 2024
24fcc0c
Update baseline commit SHA + result repo name
kocsismate Sep 24, 2024
dd0cd0e
Rename benchmark results repo after transfer
kocsismate Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 111 additions & 0 deletions .github/workflows/benchmark.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
name: Benchmark
on:
schedule:
- cron: "0 2 * * *"
permissions:
contents: read
jobs:
BENCHMARK:
name: BENCHMARK
if: github.repository == 'php/php-src'
runs-on: ubuntu-22.04
steps:
- name: Install dependencies
run: |
set -ex
sudo apt-get update
sudo apt-get install gpg

wget -O- https://apt.releases.hashicorp.com/gpg | sudo gpg --dearmor -o /usr/share/keyrings/hashicorp-archive-keyring.gpg
gpg --no-default-keyring --keyring /usr/share/keyrings/hashicorp-archive-keyring.gpg --fingerprint
echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/hashicorp-archive-keyring.gpg] https://apt.releases.hashicorp.com $(lsb_release -cs) main" | sudo tee /etc/apt/sources.list.d/hashicorp.list
sudo apt update
sudo apt install terraform=1.5.7-*
- name: Checkout benchmark suite
uses: actions/checkout@v4
with:
repository: 'kocsismate/php-version-benchmarks'
ref: 'main'
fetch-depth: 1
path: 'php-version-benchmarks'
- name: Checkout php-src
uses: actions/checkout@v4
with:
repository: 'php/php-src'
ref: 'master'
fetch-depth: 100
path: 'php-version-benchmarks/tmp/php_master'
- name: Copy-paste the same repo content for benchmarking JIT
run: |
set -e

cp -r "php-version-benchmarks/tmp/php_master/" "php-version-benchmarks/tmp/php_master_jit"
Copy link
Member

@arnaud-lb arnaud-lb Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You may want to preserve attributes such as timestamps and permissions with -a.
git clone "php-version-benchmarks/tmp/php_master/" "php-version-benchmarks/tmp/php_master_jit" would work, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the info, I didn't know this is possible. Even though I cannot think of a situation when these attributes would be important for the benchmark, I'm fine with the change as it only improves things.

- name: Setup benchmark config
run: |
set -e

cp ./php-version-benchmarks/config/infra/aws/x86_64-metal.ini.dist ./php-version-benchmarks/config/infra/aws/x86_64-metal.ini
ESCAPED_DOCKER_REGISTRY=$(printf '%s\n' "${{ secrets.PHP_VERSION_BENCHMARK_DOCKER_REGISTRY }}" | sed -e 's/[\/&]/\\&/g')
sed -i "s/INFRA_DOCKER_REGISTRY=public.ecr.aws\/abcdefgh/INFRA_DOCKER_REGISTRY=$ESCAPED_DOCKER_REGISTRY/g" ./php-version-benchmarks/config/infra/aws/x86_64-metal.ini

cp ./php-version-benchmarks/build/infrastructure/config/aws.tfvars.dist ./php-version-benchmarks/build/infrastructure/config/aws.tfvars
sed -i 's/access_key = ""/access_key = "${{ secrets.PHP_VERSION_BENCHMARK_AWS_ACCESS_KEY }}"/g' ./php-version-benchmarks/build/infrastructure/config/aws.tfvars
sed -i 's/secret_key = ""/secret_key = "${{ secrets.PHP_VERSION_BENCHMARK_AWS_SECRET_KEY }}"/g' ./php-version-benchmarks/build/infrastructure/config/aws.tfvars

cp ./php-version-benchmarks/config/php/master.ini.dist ./php-version-benchmarks/config/php/master1.ini
YESTERDAY="$(date -d "-2 day 13:00" '+%Y-%m-%d')"
Copy link
Member Author

@kocsismate kocsismate Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Intel's benchmarks years ago used a single baseline commit: https://externals.io/message/100532 They used a specific PHP 7.0 commit. I'm not sure what the best would be for us, but it may be useful to have a clear understanding about the performance of a PHP version since its development started (e.g. the baseline could be the first commit after the latest minor PHP version was branched|)

Copy link
Member Author

Choose a reason for hiding this comment

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

@arnaud-lb What do you think about this possibility? Should we add a specific commit (i.e. the first commit after PHP 8.4 is branched) to the benchmark? This way, we would compare the latest commit with the baseline and yesterday's commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've just added it, because I think it will come handy if we also have a single fixed commit against which we can compare changes.

YESTERDAY_SHA="$(cd ./php-version-benchmarks/tmp/php_master/ && git --no-pager log --until="$YESTERDAY 23:59:59" -n 1 --pretty='%H')"
Copy link
Member

Choose a reason for hiding this comment

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

A potential issue is that if this particular commit is broken or if the job is temporarily broken, we will lose some data points. E.g. if we have this:

- commit3 // master
- commit2
- commit1 // YESTERDAY_SHA

If commit3 is broken, and commit2 has a performance regression, we might not notice it.

Could we store the commit hash of the last successful benchmark in kocsismate/php-version-benchmark-results, and use that as a base?

This would also make it less likely to benchmark a commit in the middle of a merged branch.

Copy link
Member Author

@kocsismate kocsismate Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Interesting idea, and I also agree with the concern as well as the suggestion.

Another related concern is that it is currently not possible to automatically detect issues with the tests. Of course, totally broken PHP versions won't be able to run any tests, so I guess the benchmark will fail. However, if there are only smaller issues (e.g. warnings, notices or there are other kind of error) then it's possible that a different code is executed than otherwise, making the results unreliable again.

Fortunately, I've already added support for manual examination of the test outputs because I printed the response of the first request to the stdout, so one could have a look at them (I usually do a brief check). This should be extended with automatic checks for error messages, and if any is found, the benchmark should be stopped.

(tangentially related topic: I can just really hope that we will be able to use the benchmark during the development phase of PHP 9.0)

Copy link
Member Author

Choose a reason for hiding this comment

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

The benchmark now verifies the test outputs: kocsismate/php-version-benchmarks@142e907 So the only missing part is to parse the commit hash of the last successful build (which can be retrieved from the database.tsv file).

(side note: I guess I'll also have to make the database store the results per yer too, just in case)

Copy link
Member Author

Choose a reason for hiding this comment

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

The per year database storage is also implemented: kocsismate/php-version-benchmarks@0e84b9e

sed -i 's/PHP_NAME="PHP - master"/PHP_NAME="PHP - previous master"/g' ./php-version-benchmarks/config/php/master1.ini
sed -i "s/PHP_ID=php_master/PHP_ID=php_master_previous/g" ./php-version-benchmarks/config/php/master1.ini
sed -i "s/PHP_COMMIT=/PHP_COMMIT=$YESTERDAY_SHA/g" ./php-version-benchmarks/config/php/master1.ini
cp ./php-version-benchmarks/config/php/master.ini.dist ./php-version-benchmarks/config/php/master2.ini
cp ./php-version-benchmarks/config/php/master_jit.ini.dist ./php-version-benchmarks/config/php/master2_jit.ini

cp ./php-version-benchmarks/config/test/1_laravel.ini.dist ./php-version-benchmarks/config/test/1_laravel.ini
cp ./php-version-benchmarks/config/test/2_symfony_main.ini.dist ./php-version-benchmarks/config/test/2_symfony_main.ini
cp ./php-version-benchmarks/config/test/4_wordpress.ini.dist ./php-version-benchmarks/config/test/4_wordpress.ini
cp ./php-version-benchmarks/config/test/5_bench.php.ini.dist ./php-version-benchmarks/config/test/5_bench.php.ini
cp ./php-version-benchmarks/config/test/6_micro_bench.php.ini.dist ./php-version-benchmarks/config/test/6_micro_bench.php.ini

rm -rf ./php-version-benchmarks/docs/results
- name: Git setup
run: |
git config --global user.name "Benchmark"
git config --global user.email "[email protected]"
- name: Checkout benchmark results
uses: actions/checkout@v4
with:
repository: kocsismate/php-version-benchmark-results
ssh-key: ${{ secrets.PHP_VERSION_BENCHMARK_RESULTS_DEPLOY_KEY }}
path: 'php-version-benchmarks/docs/results'
- name: Run benchmark
run: ./php-version-benchmarks/benchmark.sh run aws
- name: Store results
run: |
set -ex

cd ./php-version-benchmarks/docs/results
git pull --autostash
if [ -e ".git/MERGE_HEAD" ]; then
echo "Merging, can't proceed"
exit 1
fi
git add .
Copy link
Member

Choose a reason for hiding this comment

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

This is creating one directory in the root every day. I'm not sure, but could this become an issue after some time due to the amount of entries in the root?

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting observation! I can agree with you, this is at least a threat in the future. Do you think it's enough to put the results into a single "year" directory? This way a maximum of only ~365 files would be in a directory, assuming that we will continue to run the benchmark once per day. I could also imagine a year/month/result directory structure if we want to really future proof the structure....

Copy link
Member

Choose a reason for hiding this comment

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

Yes this seems reasonable and probably enough

Copy link
Member Author

Choose a reason for hiding this comment

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

Implemented the suggestion: kocsismate/php-version-benchmarks@cc0090b

if git diff --cached --quiet; then
exit 0
fi
git commit -m "Add result for ${{ github.repository }}@${{ github.sha }}"
git push
- name: Cleanup
if: always()
run: |
set -ex

rm -rf ./php-version-benchmarks/tmp/
rm -f ./php-version-benchmarks/build/infrastructure/config/*.tfvars
rm -rf ./php-version-benchmarks/build/infrastructure/aws/.terraform/
rm -f ./php-version-benchmarks/build/infrastructure/aws/.terraform.lock.hcl
rm -f ./php-version-benchmarks/build/infrastructure/aws/aws.tfplan
rm -f ./php-version-benchmarks/build/infrastructure/aws/terraform.tfstate
rm -f ./php-version-benchmarks/build/infrastructure/aws/terraform.tfstate.backup
rm -f ./php-version-benchmarks/config/infra/aws/*.ini
Loading